parser: partially align use of loc_ghost with what the documentation says - #13854
Conversation
|
This discussion would be easier to follow if you could repeat (in your PR message or here) what the documentation says about ghost nodes -- it is documented in a somewhere obscure place (introduced in b062620) and I suspect most people are unfamiliar with this proposed specification. |
I adjusted the pr description. |
fce7e0a to
bfb73a1
Compare
Octachron
added
the
parsetree-change
| lexbuf | ||
|
|
||
| let check_loc_ghost (type a) (kind : a ast_kind) (ast : a) ~inputfile = | ||
| if Option.is_some (Sys.getenv_opt "OCAML_LOC_GHOST_CHECK") then |
There was a problem hiding this comment.
Having this check enabled on an environment variable does not sound optimal to me.
Moreover, my impression is that this check is mostly useful for the compiler, ocamlformat and ppxlib developers,
rather than most users.
Thus I think it might be better to just expose this check in the pparse API and let the compiler testsuite use it directly.
There was a problem hiding this comment.
The variable is definitely not intended to be user facing.
My thinking has been that the whole test suite should run with this, at least if the perf impact is negligible. This way, any parser change that's tested somewhere would have its ghostness checked (whereas a test dedicated to this invariant would have limited coverage, because no one would update it when updating the parser).
But this invariant doesn't hold in a number of places, so I can't really do that. So instead this is partial progress: we now have an executable specification of the invariant, but it is used only manually via the env var. With more work (which I don't currently plan to do, but may if the need arises), it could be turned on in some test, e.g. testsuite/tests/parsing, as that seems to have a good coverage of syntax.
I don't really know what I would do with a ?check_ghost_locs:bool argument in the api of pparse.
There was a problem hiding this comment.
Maybe we could have a -dparsetree-ghost-locs-invariants (or maybe -dparsetree-invariants-ghost-locs) command-line flag on compiler invocation? We already have -dcmm-invariants and -dflambda-invariants in ocamlopt.
(A cynical way to say this would be: so far we have been careful in trying not to read random environment variables, and we would want to keep some discipline there. On the other hand the command-line options are already a dumpster fire, so we could add one more user-irrelevant thing in this flat namespace.)
There was a problem hiding this comment.
We could also also have a Clflags variable (ast_linter?) without a CLI flag. Typically, the test for the AST pretty printer is already using compiler-libs, and it could easily set this new flag.
There was a problem hiding this comment.
I replaced the env var by -dparsetree-ghost-loc-invariants, and added that flag to a test that exercises a lot of the syntax I'm touching. There are still two invariants failures in there (down from a screenful, as you can see in the first commit), but we can just promote the invariant failure, no need to fix absolutely everything first.
| ; typ = | ||
| limit_quadratic_complexity (fun s -> s.typ) | ||
| (fun v -> check (fun s -> s.typ) Parse.core_type v v.ptyp_loc ) | ||
| } |
There was a problem hiding this comment.
Naive question: do you only check expressions, type expressions, and patterns because they are the most relevant or because other constructs produce less (or no) ghost locations?
There was a problem hiding this comment.
I can't say I remember my decision making, but I presumably only tested the syntactic categories I was modifying. But yes, I expect most problems (in frequency) to be in these categories.
In principle, we could test more, but it doesn't seem helpful to do that without also spending time fixing the resulting errors, otherwise I think the main effect would be more errors to sift through.
f0d955e to
e069e59
Compare
| (fun v -> | ||
| check | ||
| (* ~print:(fun f ty -> Printast.payload 0 f (PTyp ty)) *) | ||
| (fun s -> s.typ) Parse.core_type v v.ptyp_loc ) |
There was a problem hiding this comment.
Note for later: the more generic parsing function would be poly_type rather than core_type.
Octachron
left a comment
Octachron
left a comment
There was a problem hiding this comment.
The changes make the syntax keyword%ext follows the specification for ghost locations, and the test looks useful. Thus the PR looks ready to merge once:
- the menhir bootstrap are localized to a single commit
- the last debugging line is removed
|
Thanks! |
spoiler alert: it isn't
…says
Mostly in:
- the productions for keyword%extension syntaxes
- the implied 'self in `object ('self) end`
4 participants
loc.loc_ghostis supposed to false iff the ast described bylocis present at that location. The parser doesn't follow this condition consistently, which can result in unpredictable results in ocamlformat, where loc_ghost is sometimes consulted.I fixed cases created by the rules for
keyword%extensionkind of syntax, as these seems like the main offenders I've seen in ocamlformat.But there are more failures found by the check. Some may be harmless (
+foocreates a~+node whose location refers to+), some look weirder (empty non-ghost location withobject[@foo]in some case).In the changed test, the parser change should imply that a Pext_extension and its child swap their ghost boolean. But in some cases, the diff shows only ghost locations being added, which confused me. The reason is that
new%foo xhas a top node with a ghost location, while(new%foo x)creates the same top node with a non-ghost location.