-
Notifications
You must be signed in to change notification settings - Fork 715
feat: improve error messages for invalid field access #11456
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
Conversation
…ess error explination (draft)
robsimmons
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Kha can I request a spot-check on my use of the module system for non-exported error functionality here?
| module | ||
|
|
||
| prelude | ||
| import Init.Notation | ||
| import Init.Data.String | ||
|
|
||
| /-- | ||
| Translate numbers (1, 2, 3, 212, 322 ...) to ordinals with appropriate English-language names for | ||
| small ordinals (0 through 10 become "zeroth" through "tenth") and postfixes for other numbers | ||
| (212 becomes "212th" and 1292 becomes "1292nd"). | ||
| -/ | ||
| def _root_.Nat.toOrdinal : Nat -> String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lean.Elab.ErrorUtils has no public sections, so all its definitions default private
| public import Lean.Meta.Tactic.ElimInfo | ||
| public import Lean.Elab.Binders | ||
| public import Lean.Elab.RecAppSyntax | ||
| import all Lean.Elab.ErrorUtils |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using the error utils with import all
| /-- error: Unknown constant `Nat.toOrdinal` -/ | ||
| #guard_msgs in | ||
| #check Nat.toOrdinal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...and then I'm checking after the fact that I didn't accidentally add them to the Lean import
|
Mathlib CI status (docs):
|
|
Reference manual CI status:
|
|
Discussed the question I had for @Kha synchronously — the answer is that it's probably not a great idea in general to be doing the namespace polluting additions I'm doing in the current ErrorUtils, if I'm going to do them the private export + |
| Lean's field notation is very powerful, but this can also make it confusing: the expression | ||
| `color.value` can either be a single [identifier](lean-manual://section/identifiers-and-resolution), | ||
| it can be a reference to the [field of a structure](lean-manual://section/structure-fiels), and it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo in url? structure-fiel(d)s
This PR refines several error error messages, mostly involving invalid
use of field notation, generalized field notation, and numeric
projection. Provides a new error explanation for field notation.
## Error message changes
In general:
- Uses a slightly different convention for expression-type pairs, where
the expression is always given `indentExpr` and the type is given
`inlineExpr` treatment. This is something of a workaround for the fact
that the `Format` type is awkward for embedding possibly-linebreaking
expressions in not-linebreaking text, which may be a separate issue
worth addressing.
- Tries to give slightly more "why" reasoning — the environment does not
contain `String.parse`, and _therefore you can't project `.parse` from a
`String`_.
Some specific examples:
### No such projection function
```lean4
#check "".parse
```
before:
```
error: Invalid field `parse`: The environment does not contain `String.parse`
""
has type
String
```
after:
```
error: Invalid field `parse`: The environment does not contain `String.parse`, so it is not possible to project the field `parse` from an expression
""
of type `String`
```
### Type does not have the correct form
```lean4
example (x : α) := (foo x).foo
```
before:
```
error: Invalid field notation: Type is not of the form `C ...` where C is a constant
foo x
has type
α
```
after:
```
error: Invalid field notation: Field projection operates on types of the form `C ...` where C is a constant. The expression
foo x
has type `α` which does not have the necessary form.
```
## Refactoring
Includes some refactoring changes as well:
- factors out multiple uses of number (1, 2, 3, 212, 222) to ordinal
("first", "second", "third", "212th", "222nd") conversion into
Lean.Elab.ErrorUtils
- significant refactoring of `resolveLValAux` in `Lean.Elab.App` — in
place of five helper functions, a special-case function case analysis,
and a case analysis on the projection type and structure, there's now a
single case analysis on the projection type and structure. This allows
several error messages to be more explicit (there were a number of cases
where index projection was being described as field projection in an
error messages) and gave the opportunity to slightly improve positining
for several errors: field *notation* errors should appear on `foo.bar`,
but field *projection* errors should appear only on the `bar` part of
`foo.bar`.
This PR refines several error error messages, mostly involving invalid use of field notation, generalized field notation, and numeric projection. Provides a new error explanation for field notation.
Error message changes
In general:
indentExprand the type is giveninlineExprtreatment. This is something of a workaround for the fact that theFormattype is awkward for embedding possibly-linebreaking expressions in not-linebreaking text, which may be a separate issue worth addressing.String.parse, and therefore you can't project.parsefrom aString.Some specific examples:
No such projection function
before:
after:
Type does not have the correct form
before:
after:
Refactoring
Includes some refactoring changes as well:
resolveLValAuxinLean.Elab.App— in place of five helper functions, a special-case function case analysis, and a case analysis on the projection type and structure, there's now a single case analysis on the projection type and structure. This allows several error messages to be more explicit (there were a number of cases where index projection was being described as field projection in an error messages) and gave the opportunity to slightly improve positining for several errors: field notation errors should appear onfoo.bar, but field projection errors should appear only on thebarpart offoo.bar.