Skip to content
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

Do not warn about expected missing positions in quotes.reflect.Symbol #21677

Merged
merged 1 commit into from
Nov 7, 2024

Conversation

jchyb
Copy link
Contributor

@jchyb jchyb commented Sep 30, 2024

Related to #19250

In the PR above it is said that trees and symbols always have to have a position. I think certain cases of the quotes.reflect.Symbol.pos method might have been overlooked, since the doc for Symbol.span (internally in the compiler) explicitly states that the Symbol might not have a position assigned if it was not loaded from source or TASTy. It is true however for quotes.reflect.Tree.pos - the trees always have a position since they have to be loaded from source or TASTy.

Thankfully, quotes.reflect.Symbol.pos returns Option[Symbol], so we can just return None in the cases where the position does not exist. Previously we would only return None if the symbol itself did not exist, however this was not specified anywhere in the Quotes docs, so I think it's fine to change that now (or at least better then any other alternatives), even if this change would go into 3.3.5.

As a comparison, the output of println(TypeRepr.of[String].typeSymbol.pos.toString) would be:
Before #19250: Some(?) (Some(NoSource) without set span/position - can cause crashes)
After #19250: Some(?) (Some(NoSource) with span/position - can still cause crashes)
After this PR: None

Fixes #21672

@jchyb jchyb marked this pull request as draft October 1, 2024 09:36
@jchyb jchyb force-pushed the fix-i21672 branch 2 times, most recently from c38f4f0 to 352e1de Compare October 1, 2024 22:48
@jchyb jchyb marked this pull request as ready for review October 2, 2024 09:53
@jchyb jchyb force-pushed the fix-i21672 branch 3 times, most recently from a310c92 to 9af70bd Compare October 2, 2024 12:20
@jchyb jchyb requested a review from dwijnand October 3, 2024 10:06
@jchyb
Copy link
Contributor Author

jchyb commented Nov 7, 2024

@dwijnand Thank you so much for the review! I've rebased for the purpose of retesting, but haven't made any other changes since

@jchyb jchyb merged commit caac72a into scala:main Nov 7, 2024
29 checks passed
@jchyb jchyb deleted the fix-i21672 branch November 7, 2024 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Position of Some type generates warning
2 participants