fix: Account for offsets when creating new Position instances #16937
Conversation
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/58286 |
DylanPiercey
changed the title
|
Well, the FUZZ test is not running completely correctly, I will investigate. |
|
Would you like to open a PR adding Marko to out e2e test suite? https://github.com/babel/babel/blob/main/.github/workflows/e2e-tests.yml
It's that we are ignoring locations in the fuzz test so we cannot catch this, right? |
Yes! |
nicolo-ribaudo
added
the
PR: Bug Fix 🐛
|
Maybe the adjust function in those tests would adjust the locations in the AST so that we don't need to ignore them? It's been very long since I looked at that code, I don't remember well how it works. |
Amazingly correct! It's just a little more complicated to fix since we automatically adjust options in the parser. |
|
Really happy with how the tests turned out :) |
| @@ -66,8 +73,6 @@ export default function toFuzzedOptions(options) { | |||
| [randomIndex, 1, 0], | |||
| [randomIndex, randomLine, randomColumn], | |||
| [false, randomLine, false], | |||
| [false, false, randomColumn], | |||
| [randomIndex, false, false], | |||
There was a problem hiding this comment.
Can we add [randomIndex, false, randomIndex] here? It seems that this option does not overlap with others.
nicolo-ribaudo
changed the title
|
Released |
github-actions
bot
added
the
outdated
Another followup to #16936 and #16849...
new Positionin thecurPositionapi is incorrectly subtracting a "source position" (index) from an "offset position" (this.lineStart). This causes the column to be incorrect. SincelineStartis a "source position" we can subtract it directly from thepos.new Positioncalls to ensure that correctly offset line/column/index was passed and discovered theindexforfirstInvalidTemplateEscapePoswas not correctly offset.As I mentioned in the other PR I am attempting to use these new apis in Marko and with these two (hopefully final) tweaks the Marko test suite now passes. There's a fair amount of "source position" testing and snapshots in that suite so hopefully a good sign that we're not missing anything else.
Note I added another test that is a copy of all of the "start" position override tests just so that I could manually diff/compare all of the test suites from that
output.jsonto see that all the offsetting looks correct. I think it's helpful to keep that test there to reference.Again I thank all of you for working with me on this 🙏