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

Inconsistant Span of Ident inside BindingIdent #8856

Closed
caoccao opened this issue Apr 13, 2024 · 7 comments · Fixed by #8859
Closed

Inconsistant Span of Ident inside BindingIdent #8856

caoccao opened this issue Apr 13, 2024 · 7 comments · Fixed by #8859
Assignees
Labels
Milestone

Comments

@caoccao
Copy link

caoccao commented Apr 13, 2024

Describe the bug

I found the span of Ident inside BindingIdent is inconsistant. Here are 2 cases.

Case 1: BindingIdent doesn't Include TsTypeAnn

  • Parse the following code.
const a: number
  • The span of BindingIdent is as follows.
Span { lo: BytePos(7), hi: BytePos(8), ctxt: #0 }
  • The AST is as follows.
BindingIdent {
    id: Ident {
        span: Span {
            lo: BytePos(
                7,
            ),
            hi: BytePos(
                8,
            ),
            ctxt: #0,
        },
        sym: "a",
        optional: false,
    },
    type_ann: Some(
        TsTypeAnn {
            span: Span {
                lo: BytePos(
                    8,
                ),
                hi: BytePos(
                    16,
                ),
                ctxt: #0,
            },
            type_ann: TsKeywordType(
                TsKeywordType {
                    span: Span {
                        lo: BytePos(
                            10,
                        ),
                        hi: BytePos(
                            16,
                        ),
                        ctxt: #0,
                    },
                    kind: TsNumberKeyword,
                },
            ),
        },
    ),
}

Case 2: BindingIdent Includes TsTypeAnn

  • Parse the following code.
function b(a: number = 1) {}
  • The span of BindingIdent is as follows.
Span { lo: BytePos(12), hi: BytePos(21), ctxt: #0 }
  • The AST is as follows.
BindingIdent {
    id: Ident {
        span: Span {
            lo: BytePos(
                12,
            ),
            hi: BytePos(
                21,
            ),
            ctxt: #0,
        },
        sym: "a",
        optional: false,
    },
    type_ann: Some(
        TsTypeAnn {
            span: Span {
                lo: BytePos(
                    13,
                ),
                hi: BytePos(
                    21,
                ),
                ctxt: #0,
            },
            type_ann: TsKeywordType(
                TsKeywordType {
                    span: Span {
                        lo: BytePos(
                            15,
                        ),
                        hi: BytePos(
                            21,
                        ),
                        ctxt: #0,
                    },
                    kind: TsNumberKeyword,
                },
            ),
        },
    ),
}

Conclusions

  • In case 1, the BindingIdent (7, 8) doesn't include the TsTypeAnn (8, 16) and the Ident (7, 8) is expected.
  • In case 2, the BindingIdent (12, 21) includes the TsTypeAnn (13, 21) and the Ident (12, 21) is not expected because it includes the TsTypeAnn (13, 21) too.

Input code

function b(a: number = 1) {}

Config

{
  "jsc": {
    "parser": {
      "syntax": "typescript",
      "tsx": false
    },
    "target": "es2020",
    "loose": false,
    "minify": {
      "compress": false,
      "mangle": false
    }
  },
  "module": {
    "type": "es6"
  },
  "minify": false,
  "isModule": false
}

Playground link (or link to the minimal reproduction)

  • 1.3.100

https://play.swc.rs/?version=1.3.100&code=H4sIAAAAAAAAA0srzUsuyczPU0jSSLRSyCvNTUotUrBVMNRUqK4FAOamXN8cAAAA&config=H4sIAAAAAAAAA1VPOw7DIAzdcwrkuUOUoUPv0EMg6kRE%2FIRdqSjK3QsB0maz38fveRuEgJUUPMSWx7wEGQnjuWeEkmP5yQhwCkgq6sBw6yxToWZpCA9orwywjAtycSFN4zQ2BxjvCbujYVY7Paf%2FTOVtiEh0FRapdIvBa%2BLQUsH61%2Fsg2y%2Blb21wh5%2Boh52HQdOzO%2BvZ%2FQtlF66JGAEAAA%3D%3D

  • 1.4.13

https://play.swc.rs/?version=1.4.13&code=H4sIAAAAAAAAA0srzUsuyczPU0jSSLRSyCvNTUotUrBVMNRUqK4FAOamXN8cAAAA&config=H4sIAAAAAAAAA1VPOw7DIAzdcwrkuUOUoUPv0EMg6kRE%2FIRdqSjK3QsB0maz38fveRuEgJUUPMSWx7wEGQnjuWeEkmP5yQhwCkgq6sBw6yxToWZpCA9orwywjAtycSFN4zQ2BxjvCbujYVY7Paf%2FTOVtiEh0FRapdIvBa%2BLQUsH61%2Fsg2y%2Blb21wh5%2Boh52HQdOzO%2BvZ%2FQtlF66JGAEAAA%3D%3D

SWC Info output

No response

Expected behavior

The span of BindingIdent and Ident should be consistant.

  • Either the BindingIdent includes the TsTypeAnn in both cases, or the BindingIdent doesn't include the TsTypeAnn in both cases.
  • The Ident doesn't include the TsTypeAnn in both cases.

Actual behavior

No response

Version

  • 1.3.100
  • 1.4.13

Additional context

No response

@caoccao caoccao added the C-bug label Apr 13, 2024
@kdy1 kdy1 self-assigned this Apr 13, 2024
@kdy1 kdy1 added this to the Planned milestone Apr 13, 2024
kdy1 added a commit that referenced this issue Apr 15, 2024
@kdy1 kdy1 modified the milestones: Planned, v1.4.15 Apr 17, 2024
@caoccao
Copy link
Author

caoccao commented May 11, 2024

It seems this bug fix doesn't match my expectation.

The current behavior is as follows.

const a: number = 0;
// binding ident: 7, 8
//   ident: 7, 8
//   typeAnn: 10, 16

That seems to be a bit weird because the span of typeAnn is out of the span of binding ident though the typeAnn belongs to the binding ident.

My expectation is as follows.

const a: number = 0;
// binding ident: 7, 16
//   ident: 7, 8
//   typeAnn: 10, 16

@kdy1
Copy link
Member

kdy1 commented May 11, 2024

It' s not possible because BindingIdent does not have a span.

@caoccao
Copy link
Author

caoccao commented May 11, 2024

It' s not possible because BindingIdent does not have a span.

That's still confusing to me. BindingIdent supports .span(). The return of that call is weird to me. I wonder if it's possible to fix that call?

@kdy1
Copy link
Member

kdy1 commented May 11, 2024

I don't think it's good idea. BindingIdent should deref to Id and it will make .span and .span() different

@caoccao
Copy link
Author

caoccao commented May 11, 2024

Alright, thanks for the explanation.

@caoccao
Copy link
Author

caoccao commented May 12, 2024

I just fixed that at my side by this commit.

let span = if let Some(type_ann) = self.type_ann.as_ref() {
  Span {
    lo: self.id.span.lo,
    hi: type_ann.as_ref().span.hi,
    ctxt: self.id.span.ctxt,
  }
} else {
  self.span()
};

@swc-bot
Copy link
Collaborator

swc-bot commented Jun 11, 2024

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@swc-project swc-project locked as resolved and limited conversation to collaborators Jun 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging a pull request may close this issue.

3 participants