-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
CheckUnused checks span.exists before testing its parts #22504
Conversation
That's called blaming the victim. |
bb86c49
to
27224a4
Compare
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.
The only thing seems to be missing is an accompanying test case. I see that it is left as a todo
in the description, what is the status on that? Otherwise LGTM! 👍
The
The Edit: that's what I did not yet reproduce in a dotty test. I assume (without evidence) that it's a bug to see a spanless tree. I suspected tasty, special handling of By cosmic coincidence, it is the unchecked restored the other day in softwaremill/magnolia@5a030d8. |
I confirmed that |
Presumably the symptom requires more than separate compilation, but I haven't looked at what's required in a test rig. |
I will probably give up on constructing a test. I confirmed that 3.3.5 fixes the bug ( I submitted a PR to Magnolia, which is used by the failing projects. Magnolia built by 3.3.3 or 3.3.4 results in the spanless tree in For a test, I tried a macro that attempts to reset the tree span to |
Thanks for investigating! LGTM |
Must check
span.exists
before usingstart
orpoint
.Can use
pointDelta
safely.It's not obvious that certain checks for
isSynthetic
should requireexists
, since those tests will not pass for unpositioned trees.(todo)(synthetic is orthogonal to existing)Needs a test for producing unpositioned
Ident
.(todo)It's not unpositioned but spanless. The underlying bug was fixed in 3.3.5 (a macro produced an inlined annotated type tree with the symptom).Also restore alias
"unsafe-warn-patvars"
. Fix typo in other description.Fixes #22499