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

Editorial: Use grammar for VLQ and Mappings and decode via SDO #180

Merged
merged 18 commits into from
Mar 31, 2025

Conversation

szuend
Copy link
Collaborator

@szuend szuend commented Mar 10, 2025

Draft that changes VLQ and mappings to use grammar plus "syntax-directed operations" for decoding rather than a pure algorithmic approach.

Only uploaded so we have a preview for discussion.

Preview: https://szuend.github.io/source-map/branch/mappings-grammar/#sec-mappings

@nicolo-ribaudo nicolo-ribaudo self-requested a review March 10, 2025 12:40
@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Mar 10, 2025

I attempted to re-express the DecodeBase64VLQ operation without using an accumulator, since the way fields are mutated in the accumulator is not super easy to keep track of (recursion + mutable state). Do you think this version would work, or am I missing something about shifting the values?

And then you use it as "let value be the VLQSignedValue of Vlq".

VLQSignedValue ( )

Vlq :: VlqDigitList

  1. Let unsigned be the VLQUnsignedValue of VlqDigitList.
  2. If unsigned modulo 2 is 1, let sign be -1.
  3. Else, let sign be 1.
  4. Return sign × floor(unsigned / 2).

VLQUnsignedValue ( )

VlqDigitList :: DigitWithoutContinuationBit

  1. Return DecodeBase64Digit(DigitWithoutContinuationBit).

VlqDigitList :: DigitWithContinuationBit VlqDigitList

  1. Let left be DecodeBase64Digit(DigitWithContinuationBit).
  2. Let right be the VLQUnsignedValue of VlqDigitList.
  3. Return (right - 32) × 25 + left.

@szuend
Copy link
Collaborator Author

szuend commented Mar 10, 2025

Very nice! That looks much simpler. Although I think there is a bug: We need to slice the continuation bit off left and not off _right_:

3. Return _right_ × 2 ** 5 + (_left_ - 32).

Alternatively we could define DecodeBase64Digit as an SDO that handles the continuation bit.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Mar 10, 2025

You are right, the - 32 is in the wrong place 👍

I think I have a slight preference for the SDO approach, but only with a name that implies "this isn't actually the base64-decoded value, but the base64-decoded value after trimming the VLQ continuation bit" :)

Or, we remove DecodeBase64Digit, and we just add cases for the two digit types directly in the VLQUnsignedValue SDO.

@szuend
Copy link
Collaborator Author

szuend commented Mar 12, 2025

With tc39/ecmarkup#637 still in-flight, I applied Nicolos idea but via an additional nonterminal indirection. Should be good enough to discuss this in tonights' meeting.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second review pass. The VLQs look good now, and I finally reviewed the mappings definitions.

I would prefer this slightly different approach, based on the "informal" understanding that mappings contains one or more segments, one per line, separated by semicolons. A segment contains zero or more mappings, separated by commas. This avoids having to think about the "what if there are consecutive semicolons?" case, since they are just consecutive segments.

Mappings :
  Segment
  Segment `;` Mappings

Segment :
  MappingList?

MappingList :
  Mapping
  Mapping `,` MappingList

Note that the above definitions makes "mappings": "" valid, which matches the current spec.

DecodeMappingsSdo would then become
DecodeMappingsSdo ( ... )

Mappings : Segment ; Mappings
MappingList : Mapping , MappingList

  1. For each child node child of this Parse Node, do ... (same as now)

Segment : MappingList?

  1. Set state.[[GeneratedLine]] to state.[[GeneratedLine]] + 1.
  2. Set state.[[GeneratedColumn]] to 0.
  3. If MappingList is present, perform DecodeMappingsSdo of MappingList with arguments ...

Mapping : GeneratedColumn (same as now)
Mapping : GeneratedColumn OriginalSource OriginalLine OriginalColumn Name? (same as now)

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Mar 12, 2025

Maybe can we also rename Mappings to SegmentList, or MappingsSegmentList? It's a bit weird that "mappings" is a list of segments, and "segment" is a list of mappings, even though that's how we always referred to it 😅

@szuend
Copy link
Collaborator Author

szuend commented Mar 12, 2025

Thanks for the new grammar, I love it!

Maybe can we also rename Mappings to SegmentList, or MappingsSegmentList? It's a bit weird that "mappings" is a list of segments, and "segment" is a list of mappings, even though that's how we always referred to it 😅

IMO it would be nice to have consistent naming of the goal symbols w.r.t. to how the field is named in the source map JSON. What about renaming Mappings to SegmentList but then add a goal symbol MappingsField : SegmentList?

@nicolo-ribaudo
Copy link
Member

What about renaming Mappings to SegmentList but then add a goal symbol MappingsField : SegmentList?

Sounds good 👍

@szuend
Copy link
Collaborator Author

szuend commented Mar 12, 2025

Changed the grammar as per your suggestion. Also renamed the SDO to DecodeMappingsField, which makes more sense. The "Sdo" suffix was only there because I couldn't think of anything better at the time.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left minor some comments, but the rest looks good to me. I'll approve once rebased.

@nicolo-ribaudo
Copy link
Member

Oh also, per the last call we had, lets add a "Grammar notation" section in "Notational conventions". It should encourage readers to check out https://tc39.es/ecma262/#sec-grammar-notation, mentioning explicitly that:

@szuend
Copy link
Collaborator Author

szuend commented Mar 28, 2025

Oh also, per the last call we had, lets add a "Grammar notation" section in "Notational conventions".

Done. Might need some tweaking to the wording.

@szuend szuend marked this pull request as ready for review March 28, 2025 06:07
Co-authored-by: Nicolò Ribaudo <[email protected]>
@szuend szuend merged commit 6e9b07c into tc39:main Mar 31, 2025
3 checks passed
@szuend szuend deleted the mappings-grammar branch March 31, 2025 12:21
szuend added a commit to szuend/source-map that referenced this pull request Mar 31, 2025
szuend added a commit to szuend/source-map that referenced this pull request Mar 31, 2025
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.

4 participants