Should we remove the "suggestions" from parse error messages

[apblack]

A long time ago, I think as the result of a student project, many of the errors generated by the parser were accompanied by "suggestions" of possible fixes. I propose removing these suggestions.

For example:

identifierresolution.grace[1111:72]: Syntax error: a valid expression must follow '++'. This is often caused by a new line in the middle of an expression.
1110:         if (reusedScope.isLegalAsTrait.not) then {
1111:             errormessages.syntaxError ("the expression in your use " ++
-----------------------------------------------------------------------------^

Did you mean:
  1111:             errormessages.syntaxError ("the expression in your use "

Did you mean:
  1111:             errormessages.syntaxError ("the expression in your use " ++ «expression»

Did you mean:
  1111: 

That example is moderately helpful, in that the suggestions are at least syntactically correct. (The actual problem was my failure to indent the continuation line). Others are less helpful

identifierresolution.grace[160:18]: Syntax error: an elseif statement must have a condition in braces after the 'elseif'.
 159:             ast.identifierNode.new("$" ++ v, false) scope(self)
 160:         } elseif (outerChain.isEmpty) then {
-----------------------^

Did you mean:
  160:         } elseif { (outerChain.isEmpty }) then {

and

identifierresolution.grace[162:29]: Syntax error: an if statement must have 'then' after the condition in parentheses.
 161:         def v = s.variety
 162:         if (v == "dialect") || (v == "builtIn")) then {
----------------------------------^

Did you mean:
  162:         if (v == "dialect") then { || (v == "builtIn")) then {

where the suggestion is syntactic garbage.

In response to issue #243, I have already moved the spelling correction suggestions for undeclared identifiers into the error message, so these would not be affected by this proposal.

Why remove them?

• The suggestion code is a mess; it is not factored out of the main parser logic (except in a few cases where I have moved it), so it makes the parser module much longer and less maintainable.

• The suggestions are often wrong

• Students almost always use the IDE, in which case they never see them

• Highlighting the location where the parser detected the error (as with ^^^ in the above examples is much more useful; more experienced programmers (i.e., me :-) ) never look at the suggestions.

The only one that is useful is

scope.grace[239:21-22]: Syntax error: a definition must use '=' instead of ':='. A variable declaration uses 'var' and ':='.
 238:     var statusOfReusedNames := "undiscovered"
 239:     def methodTypes := dictionary.empty
--------------------------^^

Did you mean:
  239:     def methodTypes = dictionary.empty

Did you mean:
  239:     var methodTypes := dictionary.empty

but even here, the error message already says what is necessary.

[apblack]

OK, here is another one where the suggestion is actually spot-on: the problem is a missing close paren, which the suggestion hits.

identifierresolution.grace[429:28-57]: Syntax error: an argument list beginning with a '(' must end with a ')'.
 428:                 }
 429:                 entries.add(serializeVariable (v) in (s)
---------------------------------^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Did you mean:
  429:                 entries.add(serializeVariable (v) in (s))

In contrast, this one shows the downside. The problem is that I used type as part of a method name, forgetting that it is reserved. The "suggestion" is worse than useless: it actually detracts from the error message, rather than adding to it.

variables.grace[616:19]: Syntax error: a class must have a '{' after the class header.
 615: 
 616: class named(aName) type(dType) annotations (anns) kind (aKind) {
------------------------^

Did you mean:
  616: class named(aName) { type(dType) annotations (anns) kind (aKind) {

This one also makes garbage suggestions:

identifierresolution.grace[869:17-30]: Syntax error: an expression using two different operators requires parentheses
 868:         def v = vNameScope.node
 869:         if (ast.nullNode ≠ v && {v.isMethod}) then {
----------------------^^^^^^^^^^^^^^

Did you mean:
  869:         if (ast.nullNode ≠ (v && {)v.isMethod}) then {

Did you mean:
  869:         if (ast.(nullNode ≠ v) && {v.isMethod}) then {

It's possible that, with a lot of work, the suggestion system could be improved — for example, by trying to parse the suggested correction, to see if it uses at least valid context-free syntax. If we had a student who wanted to work on this, it would be a fine, self-contained project. I figure, though, that I have more important things to do with my time.

If these suggestions went away, can you conceive of anyone who would miss them?

[kjx]

I've long thought this is a symptom of a language design bug (one of the first things Michael Kölling spotted) and a mistake I still make today. in fact I made it twice will composing this email. going to = for initialisation for both defs and vars, then := for assignment, addresses the underlying problem, and even follows Wirth... J

…

On 15/01/2020, at 5:23AM, Andrew Black ***@***.***> wrote: The only one that is useful is scope.grace[239:21-22]: Syntax error: a definition must use '=' instead of ':='. A variable declaration uses 'var' and ':='. 238: var statusOfReusedNames := "undiscovered" 239: def methodTypes := dictionary.empty

[KimBruce]

A variable definition: var s: String := “hello” is equivalent to var s:String s := “hello” I can’t see any way to explain to students that the first should use “=“ rather than “:=“ but not the second! As for suggestions on errors. They are most useful when they are ~95% correct (including that they must always be syntactically correct, even if not the correct fix). The last time I taught using Grace I found them helpful, but I won’t yell too loud if they go away. Kim

…

On Jan 14, 2020, at 1:23 PM, kjx ***@***.***> wrote: I've long thought this is a symptom of a language design bug (one of the first things Michael Kölling spotted) and a mistake I still make today. in fact I made it twice will composing this email. going to = for initialisation for both defs and vars, then := for assignment, addresses the underlying problem, and even follows Wirth... J > On 15/01/2020, at 5:23AM, Andrew Black ***@***.***> wrote: > > The only one that is useful is > > scope.grace[239:21-22]: Syntax error: a definition must use '=' instead of ':='. A variable declaration uses 'var' and ':='. > 238: var statusOfReusedNames := "undiscovered" > 239: def methodTypes := dictionary.empty > — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#315?email_source=notifications&email_token=AAN2D6SXXSPOQBID7ML7SKLQ5YUOZA5CNFSM4KGV2W52YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEI6FUMA#issuecomment-574380592>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN2D6S2ZWRKPSI3P3X4M5DQ5YUOZANCNFSM4KGV2W5Q>.

[kjx]

Except of course, they're not the same. Initialisation var s: String := “hello” doesn't call the s:=(_) accessor; while var s: String; s := “hello” does. Yes those two characters ; s do change the semantics - see initasgn.grace

For novices, I can see a good argument for just banning the uninitialised var form, which makes Kim's question more-or-less moot. = defines something. := changes something.

[apblack]

@kjx, it is really hard to have a coherent discussion on one topic if you keep on diverting the discussion to a completely different topic.

Almost 2 years ago, I attempted to initiate a language design discussion on the question of whether var x and method x:=(nu) could co-exist. That discussion is about a meter long, because you diverted it to talk about whether var initialization is or is not the same as assignment.

Here, I'm asking for direction about whether I should spend time trying to fix the suggestions mechanism in minigrace, or just remove it. You have successfully diverted this conversation too.

If you think that we need to discuss the initialization vs assignment question again, what I suggest is:

1. You re-read the previous discussion, which brought up a lot of good points on both sides

2. You open a new language design issue on the gracelang language issue tracker

3. You initialize that issue by summarizing the arguments for and against, which are quite well thought-out and described.

4. You make a concrete proposal for what we should do.

[apblack]

@KimBruce wrote:

The last time I taught using Grace I found them helpful, but I won’t yell too loud if they go away.

Did you teach using the command line version of the compiler? I think that the suggestions have never shown up in the multi-file web IDE that we have been using since @zmthy deployed it in 2015.

[KimBruce]

I did not use the command line version. I must be thinking back to a much earlier version. Sorry!

…

On Jan 16, 2020, at 12:43 PM, Andrew Black ***@***.***> wrote: @KimBruce <https://github.com/KimBruce> wrote: The last time I taught using Grace I found them helpful, but I won’t yell too loud if they go away. Did you teach using the command line version of the compiler? I think that the suggestions have never shown up in the multi-file web IDE that we have been using since @zmthy <https://github.com/zmthy> deployed it in 2015. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#315?email_source=notifications&email_token=AAN2D6WGG2ZSUKBGUXEDESLQ6DBG7A5CNFSM4KGV2W52YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJFPJAI#issuecomment-575337601>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN2D6WEY3AW3QPUOIMSUXDQ6DBG7ANCNFSM4KGV2W5Q>.