UMDP3 checker incorrectly identifies .....

[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.

[t00sa]

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

[t00sa]

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

[t00sa]

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.

[t00sa]

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()

[t00sa]

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

[t00sa]

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.

[r-sharp]

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.

[ericaneininger]

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."""

[ericaneininger]

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.