Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvements in common error messages #55

Merged
merged 7 commits into from Jun 16, 2020

Conversation

inariksit
Copy link
Member

I have added clarification and debug suggestions to some of the most common error messages.

Here are GF grammars to trigger these errors: https://gist.github.com/inariksit/88281c9698fd45a5f2bd00aaf14fa183. (Note that the error message in Unsupported token gluing prints out the wrong variable due to the bug #53.)

Any suggestions and feedback are welcome!

@johnjcamilleri
Copy link
Member

Great that you've been working on this Inari! Would it be too much to ask to paste the before & after error messages here? Or as files in the error Gist/repo? It will be easier for people to comment that way.

@inariksit
Copy link
Member Author

inariksit commented Jun 5, 2020

Sure! Here are the new errors. Note that original messages are kept, I have only added clarifications. All my additions start with **.

Unsupported token gluing

Offending code:

lin unsupportedTokenGluing str = ss (str + bar") ;

Error. First line is the original, my addition starts from **

gf: 15-18: In mkSS: unsupported token gluing str + "bar"

** There was a problem in the expression `str + "bar"`
This can be due to two causes, check which one applies in your case.

1) You are trying to use + on runtime arguments. Even if you are using
   `str + "bar"` in an oper, make sure that the oper isn't called in a
   linearization that takes arguments. Both of the following are illegal:

      lin Test foo bar = foo.s + bar.s          <- explicit + in a lin
      lin Test foo bar = opWithPlus foo bar     <- the oper uses +

2) One of the arguments in `str + "bar"` is a bound variable
   from pattern matching a string, but the cases are non-exhaustive.
   Example:
      case "test" of {
        x + "a" => x + "b"   <- no applicable case for "test", so x = ???
      } ;

   You can fix this by adding a catch-all case in the end:
      { x + "a" => x + "b" ;
        _       => "default case" } ;

3) If neither applies, submit a bug report and we update the error message.
      https://github.com/GrammaticalFramework/gf-core/issues

@inariksit
Copy link
Member Author

inariksit commented Jun 5, 2020

Too many arguments

Offending GF code:

lin tooManyArgs = mkSS2 "too" "many" "args" ;
oper mkSS2 : Str -> Str -> SS = \str,dummy -> ss str ;

Error. First 2 lines are the original, my addition starts from **

Happened in linearization of tooManyArgs
   A function type is expected for mkSS2 "too" "many" instead of type {s : Str}
   ** Maybe you gave too many arguments to mkSS2

Too few arguments

Offending GF code:

lin tooFewArgs = mkSS2 "toofew" ;
oper mkSS2 : Str -> Str -> SS = \str,dummy -> ss str ;

Error. First 4 lines are original, my addition starts from **.

 Happened in linearization of tooFewArgs
    type of mkSS2 "toofew"
   expected: {s : Str}
   inferred: Str -> {s : Str}
   ** Maybe you gave too few arguments to mkSS2

Internal error in GeneratePMCFG

Offending GF code:

patternMatchingRuntime a = ss (case a.s of {"foo" => "bar"}) ;

Error. My addition starts with **.

gf: Internal error in GeneratePMCFG:
    descend (CStr 0,CNil,CProj (LIdent (Id {rawId2utf8 = "s"})) CNil)

**1) Check that you are not trying to pattern match a /runtime string/.
     These are illegal:
       lin Test foo = case foo.s of {
                         "str" => … } ;   <- explicit matching argument of a lin
       lin Test foo = opThatMatches foo   <- calling an oper that pattern matches
  2) Not about pattern matching? Submit a bug report and we update the error message.
      https://github.com/GrammaticalFramework/gf-core/issues

Mismatch in type signature: too many types in signature

Offending GF code.

oper mkSSTooManyInTypeSignature : Str -> Str -> SS = \x -> ss x ;

Error. My addition after **.

Happened in operation mkSSTooManyInTypeSignature
     type of ss x
    expected: Str -> {s : Str}
    inferred: {s : Str}
 ** Double-check that type signature and number of arguments match.

Mismatch in type signature: too few types in signature

Offending GF code.

  mkSSTooFewInTypeSignature : Str -> SS = \x,y -> ss x ;

Error. My addition after **.

Happened in operation mkSSTooFewInTypeSignature
   function type expected instead of {s : Str}
** Double-check that the type signature of the operation
   matches the number of arguments given to it.

@johnjcamilleri
Copy link
Member

Oh wow that is quite long! While very pedagogical and helpful for new users, it might be too much text to show by default. What if the default error is just something short plus a hint on how you can get the full version:

gf: 15-18: In mkSS: unsupported token gluing str + "bar"
Use the --error-help flag to display more detailed help on this error

@inariksit
Copy link
Member Author

I agree that the unsupported token gluing one is long. That's the error that pops up inappropriately (see #52 for details), so I wanted to make the error message reflect those options. Would be much better to split that message in two, and give the correct one for correct occasion.

As for --error-help flag, I would be satisfied if someone adds that, but I don't think I will have the motivation to do it. I already spent some time trying to figure out how to suppress warnings about lock fields, and didn't find how it's implemented. On the other hand, these changes that I made were very easy: just find strings in the compiler, and add more stuff to those strings.

@johnjcamilleri
Copy link
Member

On other hand adding a new flag for basically one use case isn't a good idea. Maybe the error message can link to a file in the repo hosted on GitHub. I've seen this done in other projects like linters.
Here's my attempt at condensing the error and adding a link:

There was a problem in the expression `str + "bar"`, either:
1) You are trying to use + on runtime arguments, possibly via an oper.
2) One of the arguments in `str + "bar"` is a bound variable from pattern matching a string, but the cases are non-exhaustive.
For more help see https://github.com/GrammaticalFramework/gf-core/doc/errors/gluing.md

Full details, including the suggestion to file a bug report, can then be at doc/errors/gluing.md (or whatever path seems more appropriate, maybe something under src/compiler)

@johnjcamilleri
Copy link
Member

A real world example I just came across:

$ shellcheck run.sh 

In run.sh line 35:
  VOLUME_FLAGS="${VOLUME_FLAGS} --volume \"$REPO/$P\":\"$WORKDIR/build/$P\""
                               ^----------^ SC2089: Quotes/backslashes will be treated literally. Use an array.


In run.sh line 43:
  $VOLUME_FLAGS \
  ^-----------^ SC2090: Quotes/backslashes in this variable will not be respected.
  ^-----------^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean: 
  "$VOLUME_FLAGS" \

For more information:
  https://www.shellcheck.net/wiki/SC2089 -- Quotes/backslashes will be treate...
  https://www.shellcheck.net/wiki/SC2090 -- Quotes/backslashes in this variab...
  https://www.shellcheck.net/wiki/SC2086 -- Double quote to prevent globbing ...

@inariksit
Copy link
Member Author

That's a good idea @johnjcamilleri ! I'll update my longer error messages like that, and create the documentation files.

@johnjcamilleri
Copy link
Member

I think we can merge this now?
Although I think it would be a nice longer-term goal to have documentation pages for all errors.. 🤭

@inariksit
Copy link
Member Author

Yes, I agree! In this one I just tackled the ones I found myself needing to explain the most often. :-D Aarne collected all errors last year in this file https://github.com/GrammaticalFramework/gf-core/blob/master/doc/error-messages.txt so the trouble of searching is already done.

Is there a plan for a release? I could be more motivated to document other errors if I knew that there's a deadline.

@johnjcamilleri
Copy link
Member

Aarne collected all errors last year in this file https://github.com/GrammaticalFramework/gf-core/blob/master/doc/error-messages.txt so the trouble of searching is already done.

👍

Is there a plan for a release?

It's been a while hasn't it? Traditionally there's a release before a summer school. For my part I would at least like to see that we have some updated build routines which use GitHub actions, and ideally the next release can be built & hosted directly on GitHub rather than the GF server. But I guess this discussion deserves its own thread, either here as an issue or maybe better on the GF mailing list.

@inariksit
Copy link
Member Author

Yes, that's a separate discussion.

As part of my new job, I'll be teaching GF to the other research group members, so when that becomes relevant (only 3 people have started this week, the rest will join later!), I'll get more input about which errors are confusing! But in any case, we can merge this now and just do a new PR for next improvements.

@inariksit inariksit merged commit aa9b4d0 into GrammaticalFramework:master Jun 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants