parsing: Attach a location to the RHS of Ptyp_alias #12639
Conversation
Octachron
added
the
parsetree-change
| 1 | external cast : int -> 'self nat as 'self = "%identity" | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| ^^^^ |
There was a problem hiding this comment.
I find it weird that the location does not span the quote but this is the same as in let-bindings.
I propose to change the location to include the quote. The quote is not included in the AST but is logically part of the ident as suggested by this error message:
1 | fun (x : 'a t as 'a) -> ();;
^
Error: This alias is bound to type "'a t" but is used as an instance of type "'a"
The type variable "'a" occurs inside "'a t"
There was a problem hiding this comment.
I tend to agree that it would be better to include the quote in the location.
parsing/ast_helper.ml
Outdated
| check_variable var_names t.ptyp_loc string; | ||
| Ptyp_alias(loop core_type, string) | ||
| | Ptyp_alias(core_type, alias) -> | ||
| check_variable var_names t.ptyp_loc alias.txt; |
There was a problem hiding this comment.
The variable is really the alias here, so t.ptyp_loc ⇒ alias.loc. If you want to test the change in the error message, this case is triggered by:
let none: type a. (_ as 'a) option = NoneThere was a problem hiding this comment.
Fixed in a new commit.
|
Overall, I like the change: it improves a handful of error messages and it makes the parsetree more precise on a meaningful location. I have few fine tuning remarks left however. |
Before:
1 | type t = (int as 'a) * (float as 'a)
^^^^^^^^^^^
Error: This alias is bound to type float but is used as an instance of type
int
After:
1 | type t = (int as 'a) * (float as 'a)
^
Error: This alias is bound to type float but is used as an instance of type
int
Use the location of the alias instead of the `_ as 'a` type expression:
2 | let none: type a. (_ as 'a) option = None
^
Error: In this scoped type, variable 'a is reserved for the local type a.
Co-authored-by: Florian Angeletti <florian.angeletti@inria.fr>
|
The current state looks good to me and I don't see foresee much problems for ppxlib, cc @panglesd ? |
Hi,
This patch is part of OCamlformat's extended parser, which is OCaml's parser with modifications to add missing locations and represent the different concrete syntaxes.
Merging some of these modifications back would help us keep the two parsers in sync but I think would also improve the parser for other uses (eg. error messages, PPX, etc..).
In this case, the change improves an error message.