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(parser): do not double-parse modifiers of ts index signature in class #6985

Closed
wants to merge 3 commits into from

Conversation

branchseer
Copy link
Contributor

Modifiers of ts index signatures in classes are parsed twice. The first one is ignored and the second one is always empty, causing the incorrect readonly and span in ast. https://oxc-project.github.io/oxc/playground/?code=3YCAAICvgICAgICAgICxG4jI43W9aqTWr3Wzy1UcIvgVm0uDVLP%2FHYReSN%2FJ0Mhppm0XIo%2F7DW7cd39qCwCA

Copy link

graphite-app bot commented Oct 28, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@github-actions github-actions bot added A-parser Area - Parser C-bug Category - Bug labels Oct 28, 2024
@Boshen Boshen self-assigned this Oct 28, 2024
@@ -696,6 +696,23 @@ mod test {
assert_eq!(ret.errors.first().unwrap().to_string(), "Source length exceeds 4 GiB limit");
}

#[test]
Copy link
Member

Choose a reason for hiding this comment

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

This file is for integration tests; parser tests should go into https://github.com/oxc-project/oxc/tree/main/tasks/coverage/misc/pass

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this fix can only be verified by examining the ast. It can't be done in there, can it?

@Boshen
Copy link
Member

Boshen commented Oct 28, 2024

I'm curious what project you are working on with oxc 😄

Copy link

codspeed-hq bot commented Oct 28, 2024

CodSpeed Performance Report

Merging #6985 will not alter performance

Comparing branchseer:idxsig-modifer-span (8e655d6) with main (4b450cc)

Summary

✅ 30 untouched benchmarks

@branchseer
Copy link
Contributor Author

I'm curious what project you are working on with oxc 😄

Just experimenting something 😄. Don't wanna spoil it now because I'm not sure if it's even viable. But if it is, I will surely let you guys know.

@github-actions github-actions bot added the A-ast Area - AST label Oct 28, 2024
@branchseer
Copy link
Contributor Author

Conformance tests say static is also valid for ts index signature. parser and ast updated.

× TS(1071): 'static' modifier cannot appear on an index signature.
╭─[typescript/tests/cases/conformance/classes/staticIndexSignature/staticIndexSignature4.ts:12:5]
11 │ interface IB {
12 │ static [s: string]: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you forgot to check if static is being used within an interface

@Boshen Boshen added the P-high Priority - High label Oct 30, 2024
Boshen added a commit that referenced this pull request Oct 30, 2024
@graphite-app graphite-app bot closed this in caaf00e Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ast Area - AST A-parser Area - Parser C-bug Category - Bug P-high Priority - High
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants