-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Make quotes.reflect.Position.ofMacroExpansion consider -Ymagic-offset-header
#24301
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
Conversation
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.
LGTM. But I wonder why we created a new SourcePos in the first place. I am not super familiar with the macro part, so I will want for @jchyb to approve.
|
|
||
| def context(inlinedFrom: tpd.Tree)(using Context): Context = | ||
| QuotesCache.init(ctx.fresh).setProperty(MacroExpansionPosition, SourcePosition(inlinedFrom.source, inlinedFrom.span)).setTypeAssigner(new Typer(ctx.nestingLevel + 1)).withSource(inlinedFrom.source) | ||
| QuotesCache.init(ctx.fresh).setProperty(MacroExpansionPosition, inlinedFrom.sourcePos).setTypeAssigner(new Typer(ctx.nestingLevel + 1)).withSource(inlinedFrom.source) |
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.
This is fine for the Quotes. What I don't understand is why the logic to handle the magic offset is done in sourcePos rather than in SourceFile.offsetToLine. @Linyxus?
scala3/compiler/src/dotty/tools/dotc/util/SourceFile.scala
Lines 208 to 212 in d20dd28
| def offsetToLine(offset: Int): Int = { | |
| lastLine = Util.bestFit(lineIndices, lineIndices.length, offset, lastLine) | |
| if (offset >= length) lastLine -= 1 // compensate for the sentinel | |
| lastLine | |
| } |
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.
Maybe even in SourcePosition if it is easier to manipulate the Span. sourcePos feels like a non general place to do that logic.
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.
Sounds right. I will give it a try.
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.
@Linyxus For example, we define a method sourcePos over Symbol which doesn't take into account the logic you added. If we were to report an error message where we recover the position from the symbol, it would not work.
Sounds right. I will give it a try.
Thanks!
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.
Ah I started to remember. I tried to do that initially but the main blocker is that lineToOffset and offsetToLine does not have a Context parameter, which is needed to read from compiler options. So I didn't manage to do that. Do you have an idea on how to do this properly?
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.
Right. In this case, just add the (using Context) where it is needed.
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 remember I tried that too and it turns out that this function is called where no context is available yet.
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.
We propagate it where it is needed. If calling a method is context dependent because of a setting, then it should be reflected in its signature with (using Context).
Fixes #24300
Tested manually with the reproduction in the original ticket, seems that
quotes.reflect.Position.ofMacroExpansion.startLineand.sourceFilenow correctly considers the///MAGIC:build.sbtcomment