[m-rev.] for review: ignore comments in foreign_procs for warnings

[Peter Wang]

On Tue, 27 Jan 2026 22:02:27 +1100 "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> For review by Peter, since it implements his feature request.
> The diff is with -b.
> 
> Note that this diff does not update many .err_exp[23] files that hold
> warnings expected in C# and Java grades. The reason is that
> when I implement the change I proposed last night in note
> on issue #580, those files would go away.
> 
> In case you missed it, the note says:
> 
> Would anyone object if, as part of the fix, I changed the compiler to check both the argument lists and the bodies of *all* foreign_procs, and not just of the foreign_procs that happen to be for the current backend?

No objection from me.

> Ignore (mostly) comments inside foreign_procs.
> 
> compiler/foreign.m:
>     When returning the list of identifiers inside a foreign_proc's code,
>     ignore the contents of comments. This means that warnings about
>     variable names that occur in the foreign_proc's argument list
>     but do not occur in the foreign_proc's code cannot be shut up anymore
>     by mention the variable name in comments.
> 
>     The "mostly" part is that typecheck.m still looks at the bodies
>     of foreign_procs without knowing about comments. Fixing that
>     will be part a future change.
> 
> NEWS.md:
>     Announce this breaking change.
> 
> compiler/fact_table_gen.m:
>     When we try to generate a foreign_proc's body for a fact table
>     but cannot do so due to the fact table file not being readable,
>     add a marker to the predicate that the fact table is for.
> 
> compiler/hlds_markers.m:
>     Update the documentation of the existing predicate marker
>     we use for this.
> 
> compiler/make_hlds_warn.m:
>     Do not warn about singleton variables in foreign_proc bodies
>     that are empty due to missing fact table files.
> 
> library/array.m:
> library/io.file.m:
> library/profiling_builtin.m:
> library/stm_builtin.m:
> library/table_builtin.m:
> library/term_size_prof_builtin.m:
> tests/invalid/erroneous_throw_promise.m:
> tests/invalid/foreign_procs_exist_type.m:
> tests/invalid/foreign_purity_mismatch.m:
> tests/invalid/gh72_errors.m:
> tests/invalid_purity/purity.m:
> tests/valid/solv.m:
>     Shut up warnings about singletons in foreign_procs by adding a _ prefix
>     to the names of the relevant arguments, and delete the comments
>     that can no longer do that job.
> 
> tests/invalid/foreign_purity_mismatch.err_exp:
> tests/invalid/gh72_errors.err_exp:
> tests/invalid/gh72_errors.err_exp2:
> tests/invalid/gh72_errors.err_exp3:
> tests/invalid_purity/purity.err_exp:
>     Expect updated line numbers.
> 
> tests/warnings/{warn_return.{m,err_exp}:

Delete the stray {

>     Extend this test case to test that
> 
>     - we DO warn about return statements outside comments, but
>     - we DO NOT warn about return statements inside comments.

> diff --git a/NEWS.md b/NEWS.md
> index 347e41b21..e7941663a 100644
> --- a/NEWS.md
> +++ b/NEWS.md
> @@ -70,6 +70,19 @@ Changes that may break compatibility
>    operators. (Mercury switched to using `.` as the module name separator
>    around 2003.)
>  
> +* When considering whether to generate warnings about singleton variables
> +  in `foreign_proc` pragmas, the compiler now ignores the contents of
> +  target language comments. In the past, one could shut up a warning
> +  about a variable in the argument being being unused in the foreign code
> +  by putting the variable's name in a comment. This does not work anymore.
> +  Instead, programmers should simply add an underscore to the front
> +  of the variable's name, which tells the compiler that the variable
> +  is actually intended to be unused.
> +
> +  (The upside of this new rule is that the compiler no longer generates
> +  spurious warnings about return statements in the bodies of `foreign_proc`
> +  pragmas if the word `return` appears only in comments.)
> +
>  * We have dropped support for the x86 (32-bit) version of Cygwin.
>  
>  * We have dropped support for versions of MSVC before version 19 (Visual Studio
> @@ -1613,6 +1626,19 @@ Changes to the Mercury compiler
>    C# and Java grades when a subtype type definition did not repeat the field
>    names of its base type.
>  
> +* When considering whether to generate warnings about singleton variables
> +  in `foreign_proc` pragmas, the compiler now ignores the contents of
> +  target language comments. In the past, one could shut up a warning
> +  about a variable in the argument being being unused in the foreign code
> +  by putting the variable's name in a comment. This does not work anymore.
> +  Instead, programmers should simply add an underscore to the front
> +  of the variable's name, which tells the compiler that the variable
> +  is actually intended to be unused.
> +
> +  (The upside of this new rule is that the compiler no longer generates
> +  spurious warnings about return statements in the bodies of `foreign_proc`
> +  pragmas if the word `return` appears only in comments.)
> +
>  * We have improved the typechecking and modechecking of type conversion
>    expressions.
>  

A bit shorter:

* The compiler now ignores comments when warning about unused arguments
  in `foreign_proc` pragmas. Warnings can no longer be suppressed by
  mentioning a variable in a comment; use a leading underscore to mark
  intentionally unused variables. This change also prevents spurious
  warnings about `return` statements caused by the word `return`
  appearing only in comments.

> diff --git a/compiler/fact_table_gen.m b/compiler/fact_table_gen.m
> index 54eca5a0b..e26fa18c5 100644
> --- a/compiler/fact_table_gen.m
> +++ b/compiler/fact_table_gen.m
> @@ -632,6 +633,12 @@ fact_table_compile_facts(ProgressStream, ModuleInfo, FactTableFileName,
>          io.close_input(FactTableFileStream, !IO)
>      ;
>          FactTableFileResult = error(Error),
> +
> +        pred_info_get_markers(!.PredInfo, PredMarkers0),
> +        add_marker(marker_fact_table_semantic_errors,
> +        PredMarkers0, PredMarkers),
> +        pred_info_set_markers(PredMarkers, !PredInfo),

Fix the indentation.

> @@ -773,6 +780,12 @@ compile_fact_table_in_file(MaybeProgressStream, FileStream, FileName,
>      ;
>          OpenCompileSpecs = [_ | _],
>          !:Specs = OpenCompileSpecs ++ !.Specs,
> +
> +        pred_info_get_markers(!.PredInfo, PredMarkers0),
> +        add_marker(marker_fact_table_semantic_errors,
> +        PredMarkers0, PredMarkers),
> +        pred_info_set_markers(PredMarkers, !PredInfo),

Fix the indentation.

> diff --git a/tests/warnings/warn_return.m b/tests/warnings/warn_return.m
> index 52e3c822b..b7d1a3574 100644
> --- a/tests/warnings/warn_return.m
> +++ b/tests/warnings/warn_return.m
> @@ -37,4 +36,26 @@
>      return;
>  ").
>  
> +:- pragma foreign_proc("C",
> +    bar(X::in, Y::out),
> +    [will_not_call_mercury, promise_pure],
> +"
> +    X = Y;
> +    // return;
> +").

I suggest changing one of these to /* */ style.

> +:- pragma foreign_proc("C#",
> +    bar(X::in, Y::out),
> +    [will_not_call_mercury, promise_pure],
> +"
> +    X = Y;
> +    // return;
> +").
> +:- pragma foreign_proc("Java",
> +    bar(X::in, Y::out),
> +    [will_not_call_mercury, promise_pure],
> +"
> +    X = Y;
> +    // return;
> +").
> +

The rest looks fine. Thanks.

Peter