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

Fix #14240: Fix off-by-1 when emitting Scala.js IR Positions. #14318

Merged
merged 1 commit into from
Jan 26, 2022

Conversation

sjrd
Copy link
Member

@sjrd sjrd commented Jan 21, 2022

scalac positions are 1-based, while IR positions are 0-based. There is therefore a - 1 in the nsc plugin, that was ported over to dotc without questioning. It turns out that dotc uses 0-based positions as well (as documented in SourceFile), and so we should not make any adaptation there.


I'm not sure how to write a test for this.

@sjrd
Copy link
Member Author

sjrd commented Jan 25, 2022

@anatoliykmetyuk Hum, why did you assign me to my own PR? Do I need to change anything here?

@anatoliykmetyuk
Copy link
Contributor

@sjrd is this one review-ready or did you intend to also write the test?

@sjrd
Copy link
Member Author

sjrd commented Jan 25, 2022

Unless someone has any idea how we can write that programmatically calls the compiler, with the -scalajs flag, extracts the IR that it produces, and checks positions on it ... hum, no, I don't plan to write any test about it.

@smarter
Copy link
Member

smarter commented Jan 25, 2022

Could this be done as an sbt scripted test? We have one for scalajs already: https://github.com/lampepfl/dotty/tree/master/sbt-test/scalajs

@sjrd
Copy link
Member Author

sjrd commented Jan 25, 2022

It's possible, sure. If you think that's not too much overhead.

@smarter
Copy link
Member

smarter commented Jan 25, 2022

The scripted tests run reasonably fast nowadays so I think so, you could even replace the existing "basic" test.

scalac positions are 1-based, while IR positions are 0-based. There
is therefore a `- 1` in the nsc plugin, that was ported over to
dotc without questioning. It turns out that dotc uses 0-based
positions as well (as documented in `SourceFile`), and so we should
not make any adaptation there.
@sjrd sjrd force-pushed the sjs-fix-ir-positions branch from 528fd8e to 2b76faa Compare January 26, 2022 11:24
@sjrd
Copy link
Member Author

sjrd commented Jan 26, 2022

All right. I updated the PR with a test in the existing scripted test.

@sjrd sjrd requested a review from smarter January 26, 2022 15:24
@sjrd sjrd assigned smarter and unassigned sjrd Jan 26, 2022
@smarter smarter merged commit dc24b74 into scala:master Jan 26, 2022
@smarter smarter deleted the sjs-fix-ir-positions branch January 26, 2022 15:27
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.

3 participants