[m-rev.] for post-commit review: fix misleading context in error message for Sean's code

[Zoltan Somogyi]

On 2023-11-05 15:14 +11:00 AEDT, "Julien Fischer" <jfischer at opturion.com> wrote:
>>              WarningSpecs = CondWarningSpecs ++ ThenWarningSpecs,
>> +            % There is else part in the source code, so any context
> 
> There is *no* else part.

Added.

>> +            % we use for it will be misleading. Using Context here is
>> +            % also misleading, but any other context from the source code
>> +            % will be at least as bad.
>> +            % XXX We could use a context that is NOT in the source code.
>> +            % For example, we could use the line number from Context,
>> +            % but replace the filename with something like "MISSING_ELSE".
> 
> I think the only way you are reasonably going to do any better here is by
> recording somewhere that the else branch arises from the DCG transformation and
> mentioning that fact in the error message.  I suspect the result of your suggestion
> in the XXX comment on the part of users will simply be: "Huh, what's that?".
> 
> The diff is fine otherwise.

Thanks. I committed this diff to address the above.

Zoltan.

--- a/compiler/parse_dcg_goal.m
+++ b/compiler/parse_dcg_goal.m
@@ -834,13 +834,31 @@ parse_dcg_goal_if(ArgTerms, Context, ContextPieces,
             MaybeThenGoal = ok2(ThenGoal, ThenWarningSpecs)
         then
             WarningSpecs = CondWarningSpecs ++ ThenWarningSpecs,
-            % There is else part in the source code, so any context
+            % There is no else part in the source code, so any context
             % we use for it will be misleading. Using Context here is
             % also misleading, but any other context from the source code
             % will be at least as bad.
             % XXX We could use a context that is NOT in the source code.
             % For example, we could use the line number from Context,
             % but replace the filename with something like "MISSING_ELSE".
+            %
+            % Neither I (zs) nor juliensf like that. Julien proposed,
+            % on m-rev on 2023 nov 5, that we add a goal_feature that
+            % marks ElseGoal as being compiler generated, so that any
+            % messages about e.g. mode errors involving ElseGoal can mention
+            % the fact that this goal is generated by the compiler for
+            % this missing else. However, implementing this suggestion
+            % would take a large amount of work. For starters, goal_features
+            % are HLDS constructs, so we cannot attach them to ElseGoal
+            % directly; we would need some other mechanism, such as an extra
+            % flag argument in the if_then_else_expr below, to convey this
+            % info to the code in goal_expr_to_goal.m. And then we would
+            % have to modify every piece of code that generates error messages
+            % about goals that may have been created here (a) to test for
+            % this new goal_feature, and (b) to add this reminder if the
+            % feature is there. That would be a huge amount of work,
+            % which is not warranted for improved diagnostics for errors
+            % in a rarely-used construct.
             ElseContext = Context,
             ( if OutVar = InVar then
                 ElseGoal = true_expr(ElseContext)