-
Notifications
You must be signed in to change notification settings - Fork 713
fix: do not modify infotrees in withSetOptionIn
#11313
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
base: master
Are you sure you want to change the base?
fix: do not modify infotrees in withSetOptionIn
#11313
Conversation
|
changelog-language |
|
awaiting-review |
|
Reference manual CI status:
|
|
Mathlib CI status (docs):
|
Kha
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.
I think we also noticed that existing core linters worked around this, could you adjust them as well (and check that their behavior is covered by a test)?
| Given a command elaborator `cmd`, returns a new command elaborator that | ||
| first evaluates any local `set_option ... in ...` clauses and then invokes `cmd` on what remains. | ||
| This is expected to be used in linters, after elaboration is complete. It is not appropriate for |
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.
Can we move it to Linter.Basic then?
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.
Hmm, I'm having difficulty doing so without introducing a build cycle.
withSetOptionIn uses definitions in Lean.Elab.Command, but Lean.Elab.Command depends on Lean.Elab.Binders, which depends on Lean.Linter.Basic (only) in order to log a deprecation for let_fun in elabLetFunDecl. (Haven't checked if that's the only dependency Lean.Linter.Basic → Lean.Elab.Command.)
Should we leave withSetOptionIn in Lean.Elab.Command for now, or can we shuffle things around somehow?
(I've reverted my failing attempts to move it for now.)
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.
Hmm, Linter.Util then?
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.
Thought about this a bit:
- I feel that ideally
Lean.Linter.Basicshould give you everything you need to define a typical linter.withSetOptionInought to be part of that, as it's only an unusual linter which avoids it. - Moreover,
Lean.Elab.Commandshould also be part of that, as it hasaddLinter, which you will always want. So it would be nice ifLean.Linter.Basiccould actually publicly importLean.Elab.Command.
Assuming let_fun is the only issue (I'm testing this at #11497), what do you say to either (a) removing let_fun (seems it was deprecated 5 months ago, June 29; not sure if that's long enough) or (b) moving it somewhere else? (The option logic for logLintIf seems inextricable from Linter.Basic, so inlining that code isn't easy. We could of course issue a "worse" deprecation warning that just checks the option directly; I'm not sure if anyone is likely to be relying on linter sets/linter.all to turn off that warning...)
It might make sense to do this in another couple of PRs, of course. :)
EDIT: It seems more things prior to Lean.Elab.Command log deprecation warnings than let_fun. :( It would be nice to have one module that people import to write a linter, though...Lean.Linter seems to also include actual linters, and I imagine would therefore not be most people's first choice; Util connotes optional utilities, not the basic infrastructure. Either way it does seem that the contents of Lean.Linter.Basic have to precede Lean.Elab.Command, though.
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.
What do you think about:
- moving the contents of
Lean.Linter.Basicto a new file, e.g.Lean.Linter.Log(orLean.Linter.Option,Lean.Linter.Init, or something else) - putting
withSetOptionInin the now-emptyLean.Linter.Basic, and publicly importingLean.Elab.Commandthere.
Or, of course, we can just make Lean.Linter.Util the file to import when writing linters. I'm willing to make either of these changes, ultimately; I just have a preference for providing all the basic linter necessities in Lean.Linter.Basic (which includes Lean.Elab.Command and addLinter) if possible instead. :)
This reverts commit c30303e.
This PR ensures that
withSetOptionIndoes not modify the infotrees or error on malformed option values, and thus avoids panics in linters that traverse the infotrees withvisitM.withSetOptionInis only intended to be used in linters, and thus should provide the linter action with the infotrees produced during elaboration without modification. However,withSetOptionInhad not only been modifying the infotrees by elaborating the option, but had been producing context-free info nodes in doing so. These caused uses ofvisitMand related functions to panic in typical linters.Likewise,
withSetOptionInalso should not cause the linter to error if somehow it consumes a malformed option value, but instead should fail silently.We give an optional flag
(addInfo := true)toElab.elabSetOptionwhich controls whether info is added to the infotrees.This was brought up on Zulip here, and discussed briefly in office hours.