You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This is an alternative fix to #14769.
We change slightly the representation of Texpand nodes, to keep track explicitly of the level at which abbreviations have been checked.
This should also avoid checking the same abbreviation over and over again (across separate calls to update_level).
I looked at this out of curiosity and I have naive questions (showing that maybe I should have looked at the Texpand patch more):
What is the intended semantics of the level field of the Texpand description, and how does it differ from the level of the corresponding type_expr node? (Bonus question: could this maybe be documented somewhere?)
Why do we have check_level_type in the first place for Texpand nodes? I see
andupdate_level_abbrevenvlevelexpandty=
iter_abbrev
(funpargs ->
if level <Path.scope p then forget_abbrev ty elseifList.for_all (check_level_type level) args then()elseif expand || needs_expand env level p args then forget_abbrev ty elseList.iter (update_level env level expand) args)
ty
Why is it not enough to do
if expand || level <Path.scope p || needs_expand env level p args
then forget_abbrev ty
elseList.iter (update_level env level expand) args
Another naive question: unless I am missing something, update_level does not update the level of the result of the expansion? I find this a bit unintuitive. Are the level invariants expected from Texpand explained somewhere?
I have added comments to check_level_type and update_abbrev_level.
The second one is lengthy, because this is a bit tricky.
As for your second question, in the two expansion cases (Tconstr), update_level is called on the result of the expansion: after link_type ty ty', ty and ty' share the same representative node (and level), the only difference being that ty may contain an abbreviation, which we want to check also.
There is no change in the invariants expected: levels should be decreasing when going from parent to child, the abbreviation also counting as child for the parent node. As before, levels on nodes Tlink or Texpand are ignored.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is an alternative fix to #14769.
We change slightly the representation of
Texpandnodes, to keep track explicitly of the level at which abbreviations have been checked.This should also avoid checking the same abbreviation over and over again (across separate calls to
update_level).