Skip to content

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 1, 2025
Merged

Make quotes.reflect.Position.ofMacroExpansion consider -Ymagic-offset-header #24301

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/quoted/MacroExpansion.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,6 @@ object MacroExpansion {
ctx.property(MacroExpansionPosition)

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)
Copy link
Member

@hamzaremmal hamzaremmal Nov 1, 2025

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
}

Copy link
Member

@hamzaremmal hamzaremmal Nov 1, 2025

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.

Copy link
Contributor

@Linyxus Linyxus Nov 1, 2025

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.

Copy link
Member

@hamzaremmal hamzaremmal Nov 1, 2025

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!

Copy link
Contributor

@Linyxus Linyxus Nov 1, 2025

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?

Copy link
Member

@hamzaremmal hamzaremmal Nov 1, 2025 •
edited
Loading

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.

Copy link
Contributor

@Linyxus Linyxus Nov 1, 2025

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.

Copy link
Member

@hamzaremmal hamzaremmal Nov 1, 2025

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).

}

Loading