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.

Sign up for GitHub

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

Jump to bottom

Spelling errors in pattern matches #240

Open
heatherleaf opened this issue May 15, 2019 · 12 comments
Open

Spelling errors in pattern matches #240

heatherleaf opened this issue May 15, 2019 · 12 comments

Comments

@heatherleaf
Copy link
Member

GF doesn't make any syntactic distinction between how parameters, functions and variables are written. There is a convention that parameters should be capitalised, but functions and variables be lower-cased, but it's not enforced. This leads to problems, here is an example:

abstract TestAbs = {
cat S; B;
fun sOk, sBug : B -> B -> S;
fun bTrue, bFalse : B;
}
concrete Test of TestAbs = {
param Bool = True | False;
lincat S = {s : Str};
lincat B = {s : Str; b : Bool};
lin sOk x y = {s = x.s ++ y.s ++ case <x.b, y.b> of {
        <True, False> => "<TrueFalse>" ;
        <_,    _    > => "" 
}};
lin sBug x y = {s = x.s ++ y.s ++ case <x.b, y.b> of {
        <True, Fasle> => "<TrueFasle>" ;
        <_,    _    > => "" 
}};
lin bTrue  = {s = "True"; b = True};
lin bFalse = {s = "False"; b = False};
}

The only difference is that False is misspelled in sBug. Here's the result:

> i -src Test.gf
- compiling TestAbs.gf...   write file TestAbs.gfo
- compiling Test.gf...   write file Test.gfo
linking ... OK

TestAbs> gt sOk ? ? | l
False False
False True
True False <TrueFalse>
True True

TestAbs> gt sBug ? ? | l
False False
False True
True False <TrueFasle>
True True <TrueFasle>

The misspelling of Fasle makes it a parameter variable which then captures <True,True> too, which was not intended. This bug is very difficult to catch, because GF didn't print any warning at all.

How can we capture these errors better?
@heatherleaf
Copy link
Member Author

I have a solution too! 🥇

I don't suggest the Haskell way: to require parameters to be capitalised and variables lower-cased. But I suggest that we make it explicitly recommended:

  • Print a warning whenever a parameter is defined non-capitalised, and whenever a variable is capitalised. (Let functions be either, because the RGL is sprinkled with capitalised functions)
  • (Optional) Add a per-module compiler flag for suppressing these warnings (can be useful for old modules that do not follow the recommendation)

@johnjcamilleri
Copy link
Member

I like the idea of warnings to encourage conventions.
Are you saying this should be implemented in the compiler for all GF grammars, or somehow targeted only at the RGL?

Rather than build this into the compiler, we could also imagine a separate gflint tool. This is very common with other languages, eg. HLint for Haskell. Implementing this shouldn't be too hard using GF-as-a-library and having it separate from the compiler is probably good from a separation of concerns point of view. Not least because it's recently been discussed that the GF error messages need a bit of an overhaul.

@heatherleaf
Copy link
Member Author

I like the idea of warnings to encourage conventions.
Are you saying this should be implemented in the compiler for all GF grammars, or somehow targeted only at the RGL?

This is dangerous and hard-to-catch for all GF grammars, not just the RGL

Rather than build this into the compiler, we could also imagine a separate gflint tool.

That's also a possibility of course. But I think this is more severe than many of the current warnings that the compiler generates (e.g., about missing lock fields), so either move all warnings to the gflint tool, or have them in the compiler. Whatever has the highest chance of getting implemented :)

@johnjcamilleri
Copy link
Member

You're saying this is "severe" as if it's a bug in GF, but in truth it is a feature of the language. That it can lead to hard to find errors is just an unfortunate side-effect.
I don't agree that a warning about this is "more serious" than other GF warnings.

@heatherleaf
Copy link
Member Author

You're saying this is "severe" as if it's a bug in GF, but in truth it is a feature of the language. That it can lead to hard to find errors is just an unfortunate side-effect.

Let's say it's a very unfortunate feature of the language...

I don't agree that a warning about this is "more serious" than other GF warnings.

This is of course debatable and I guess that discussion won't lead to anything useful. Let me rephrase to:

The warning I suggest is as important as the current warnings that the compiler generates, because it can catch serious grammar bugs just as the current warnings can. (E.g., "patterns never reached", "no linearization of X", "function X is not in abstract", "missing lock field")

@aarneranta
Copy link
Contributor

aarneranta commented May 16, 2019 via email

@aarneranta
Copy link
Contributor

aarneranta commented May 16, 2019 via email

@heatherleaf
Copy link
Member Author

Now I see that the overshadowing warning is already there ;-) Maybe we should just promote this to an important warning and tune down some less important ones (e.g. lock fields). This could be done by using verbosity levels.

The problem with my example is that it doesn't trigger the overshadowing warning! (That's why I had to match over a pair of bools)

The misspelled Fasle just "steals" one of the three possible alternatives that would otherwise go to the catch-all. So currently, my example grammar doesn't generate any warning at all.

@heatherleaf
Copy link
Member Author

I'm perfectly ok with a more complicated heuristics for generating warnings. Here is another alternative:

  • (my original suggestion) warn if a parameter is defined lowercased, and if a variable is upper-cased
  • (alternative) warn if a variable which doesn't start with _ is not used

One reason I suggested the first one is that it's probably extremely easy to implement. Just do a simple string check whenever a parameter is defined, and whenever a variable is created (in a lambda or a pattern match).

@aarneranta
Copy link
Contributor

aarneranta commented May 16, 2019 via email

@aarneranta
Copy link
Contributor

aarneranta commented May 16, 2019 via email

@heatherleaf
Copy link
Member Author

Sorry for a mistake in my previous comment: this is not a bug, because the pattern <,> is reached by <False,True> for instance. Hence the overshadowing analysis is not sufficient to capture errors like this.

Exactly!

Perhaps my second suggestion is better? To issue a warning if a (named) variable is not used. That would capture most misspellings (unless you misspell the same thing twice)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@johnjcamilleri @heatherleaf @aarneranta