Skip to content

UMDP3 checker incorrectly identifies ..... #235

Open

Description

@r-sharp

This is a location for documenting either false failure flags or missing failure flags from the UMDP3 checker.
It is intended to inform the follow on PR from #234

At the time of writing, the only false failures spotted so far ar the incorrect labelling of the variable 'long' as a deprecated intrinsic. Given that variable names shouldn't be the same as intrinsics anyway, this is morst likely to require a change top the UM's code, but is listed here in case a solution at this end materializes.

Activity

  1. t00sa commented on May 19, 2026

    @t00sa
    Collaborator

    This is a false positive:

    ✗ FAIL  file : um/src/atmosphere/gravity_wave_drag/g_wave5a.F90
        --------------------------------------------------------------------------------------
         Unseparated Keywords                                         : Found   1 failure.
            ----------------------------------------------------------------------------------
             1 : unseparated keyword in line: REAL(KIND=real_umphys), INTENT(IN OUT) ::                &!intent(inout): : [237]
            ----------------------------------------------------------------------------------
    

    The bit of code responsible is as follows and removing the comment after the continuation character makes the problem go away:

    REAL(KIND=real_umphys) ::               &!intent(inout):
      r_u(udims_s%i_start:udims_s%i_end,    & !u wind increment diagnostic
  2. t00sa commented on May 19, 2026

    @t00sa
    Collaborator

    Preprocessor macros are being picked up as lowercase or unseparated keywords:

    ✗ FAIL  file : um/src/io_services/common/geometry/ios_model_geometry.F90
        --------------------------------------------------------------------------------------
         Capitalised Keywords                                         : Found   1 failure.
            ----------------------------------------------------------------------------------
             1 : lowercase keyword: if : [262]
            ----------------------------------------------------------------------------------
         Unseparated Keywords                                         : Found   1 failure.
            ----------------------------------------------------------------------------------
             1 : unseparated keyword in line: #endif : [270]
            ----------------------------------------------------------------------------------
    

    Relevant section of code is:

    #if !defined(CPP_IOS_STATIC_DATA)
    ! Allocate sendbuffers.
    DO ibuf=1,IOS_AsyncNumSlots
      ALLOCATE(slot(ibuf)%buffer (MaxFieldDomain*IOS_AsyncMaxFieldsInPack))
      ALLOCATE(slot(ibuf)%control(                                                 &
          ios_max_cntrlwords_per_field *                                           &
          IOS_AsyncMaxFieldsInPack))
    END DO
    #endif
  3. t00sa commented on May 19, 2026

    @t00sa
    Collaborator

    Also, I'm not sure if this is a false positive or not: a comment line over 80 characters getting picked up as a long line.

  4. t00sa commented on May 19, 2026

    @t00sa
    Collaborator

    I think this is a known bug where .TRUE. or .FALSE. trips up the uppercase variable checker, e.g. in um/src/atmosphere/dynamics/conservation_diag_mod.F90

    LOGICAL, PARAMETER :: initial_timestep = .TRUE.
    LOGICAL, PARAMETER :: not_initial_timestep = .FALSE.

    This also causes false positives:

    INTEGER, PARAMETER :: defaultsize = BIT_SIZE(0)/8

    Also:

    INTEGER, POINTER     :: atm_global_points(:,:)=>NULL()
  5. t00sa commented on May 19, 2026

    @t00sa
    Collaborator

    This one is just a diagnostic message mismatch - the error reported is UPPERCASE variable name even though the variable is mixed case:

    REAL, INTENT(IN OUT) :: STASHwork14(:)   ! STASH workspace
  6. t00sa commented on May 19, 2026

    @t00sa
    Collaborator

    A possible solution to both the .TRUE. etc problem and the camel case one - although, in this definition variables like FOO_BAR_BAZ are accepted, which might not be what we want.

    The change to lowercase_variable_names() in umdp3_checker_rules.py is as follows:

    clean_line = re.sub(
        r"^\s*(INTEGER|REAL|LOGICAL|CHARACTER|TYPE)\s*.*::\s*",
        "",
        clean_line,
    )
    
    # Remove everything after any equals sign
    clean_line = clean_line.split("=")[0]
    
    if re.search(r"\b(?>[A-Z]{2,})(?![_\w]+)", clean_line):
        self.add_extra_error("UPPERCASE variable name")
        failures += 1
        error_log = self.add_error_log(
            error_log, "UPPERCASE variable name", count + 1
        )

    It strips off everything after an = sign to deal with logicals, functions, null pointers etc, and then uses a pattern which matches something that starts with two upper case characters and, if that matches, only matches if it is not followed by an underscore or a word character. This seems to work in most - all? - cases. The only snag is that the (?>...) syntax requires python 3.11.

  7. r-sharp commented on May 21, 2026

    @r-sharp
    ContributorAuthor

    Sam Clarke-Green (@t00sa) - we may need to have a chat. Are you getting these errors with the version implemented by #234 ?
    That change is supposed to have fixed :

    • code styling failures seen in comments at the ends of lines
    • preprocessor macros being mis-identified as Fortran intrinsics
    • Errors (such as .TRUE. and .FALSE.) being found to right of an "=" where assingnment is made during declaration.

    Also, in the code I'm looking at there is no longer has a method called lowercase_variable_names in umdp3_checker_rules.py. I think that was removed by the change previous to #234

    The replacement to lowercase_variable_names is r3_4_2_no_full_uppercase_variable_names in umdp3_rules_S3.py and among other fixes it broadens what is accepted to anything where, having trimmed anything left of ::, split on ,s and then removed anything to the right of =s, it checks to see if variable.upper() != variable where true is considered a pass. So where the previous iteration tried to be more of a CamelCase check where 2 or more adjacent uppercase letters would be considered a fail - the current just abhors anything fully uppercase. Which is much more in keeping with the stuff we see in the UM such as REAL, INTENT(IN OUT) :: STASHwork14(:) ! STASH workspace

    The one's I'll have to double check are if =>NULL and BIT_SIZE(0)/8 still give it problems, which makes me "oh so grateful" for unit tests and the fact that you've posted the real lines that have issues.

  8. ericaneininger commented on Jun 11, 2026

    @ericaneininger
    Contributor

    The following is erroneously reported as an uppercase variable name.

        --------------------------------------------------------------------------------------
    ✗ FAIL  file : /home/users/erica.neininger/cylc-run/rose-stem/run1/share/source/um/src/atmosphere/nudging/nudging_input_mod.F90
        --------------------------------------------------------------------------------------
         No Full Uppercase variable names                             : Found   1 failure.
        --------------------------------------------------------------------------------------
    
    CHARACTER(LEN=30)   :: ndg_filename_template(2)= ['unset','unset']

    The reason is that quoted strings are removed, which leaves the variable name (when clean_line is split by ,) as "]". This is evaluated as uppercase. Could be eliminated by checking that the variable name contains actual letter characters.

    Relevant code snippets from umdp3_rules.s3.py :

    def r3_4_2_no_full_uppercase_variable_names(lines: List[str]) -> TestResult:
    ...
                variables = [var.strip() for var in clean_line.split(",")]
                for var in variables:
                    var = var.split(r"=", 1)[0].strip()  # Remove any assignment part
                    if var.upper() == var:
                        failures += 1

    ...Alternatively, this may be the reason queried by the comment in the code:

    def remove_quoted(line: str) -> str:
        """Remove quoted strings from a line"""
        """
        TODO: The original version replaced the quoted sections with a
        "blessed reference", presumably becuase they were 're-inserted' at some
        stage. No idea if that capability is still required."""
  9. ericaneininger commented on Jun 11, 2026

    @ericaneininger
    Contributor

    The following are erroneously reported as an uppercase variable names.

        --------------------------------------------------------------------------------------
    ✗ FAIL  file : /home/users/erica.neininger/cylc-run/rose-stem/run1/share/source/um/src/atmosphere/nudging/nudging_main1-nudging_main1.F90
        --------------------------------------------------------------------------------------
         No Full Uppercase variable names                             : Found   2 failures.
        --------------------------------------------------------------------------------------
    

    At line 154:

    REAL(KIND=real_umphys), INTENT(IN OUT) ::                                      &
    !
     u( udims_s%i_start : udims_s%i_end,                                           &

    This is due to a failure to concatenate lines where there is a comment line in the middle - resulting in the full line being evaluated as:
    REAL(KIND=real_umphys), INTENT(IN OUT) :: and therefore there the variable name detected is an empty string.

    A possible fix is to add in an extra while True loop in the concatenate_lines method - such as:

    def concatenate_lines(lines: List[str], line_no: int) -> str:
    ...
        while line.rstrip().endswith("&"):
            line = line.rstrip()[:-1]  # Remove the continuation character
            while True:
                # Loop over lines until the next_line is not a comment
                line_no += 1
                if line_no > len(lines):
                    break  # Avoid going out of bounds
                next_line = lines[line_no - 1]
                if not next_line.lstrip().startswith('!'):
                    break

    This will fix the imminent problem, but we've lost the out-of-bounds break out of the original while loop - so it's not a complete solution.

    Similarly, at L174:

    REAL(KIND=real_umphys), INTENT(IN) ::                                          &
    !
     p_theta_levels( tdims_s%i_start : tdims_s%i_end,                              &

    Another fix, if we're happy that concatenate_lines is not actually doing its job, is to simply check that the the variable being checked is not a zero length string.

  10. r-sharp commented on Oct 16, 2026

    @r-sharp
    ContributorAuthor

    I have added a couple of Sam's examples and both of Erica's as tests in the unit tests for 'r3_4_2_no_full_uppercase_variable_names' - None of them register as a failure.
    Similarly, I've run the script in "full check" mode on the parent directories of the files indicated in the comments. (can't currently request checks on a single file)
    None of the files indicated are throwing the errors listed here (others, yes, mostly long lines, unlabelled exit statements and multiple program units, but not 'No Full Uppercase variable names'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions