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(isolated_declarations): Infer type of template literal expressions as string #4068

Conversation

MichaelMitchell-at
Copy link
Contributor

In a "non-const" context, a template literal string can just be inferred a string. This is consistent with TypeScript's behavior.

Copy link

graphite-app bot commented Jul 6, 2024

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

Add the label “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 the A-isolated-declarations Isolated Declarations label Jul 6, 2024
@MichaelMitchell-at MichaelMitchell-at force-pushed the 07-06-fix_isolated_declarations_infer_type_of_template_literal_expressions_as_string branch from 2b96215 to d920382 Compare July 6, 2024 09:58
Copy link

codspeed-hq bot commented Jul 6, 2024

CodSpeed Performance Report

Merging #4068 will not alter performance

Comparing MichaelMitchell-at:07-06-fix_isolated_declarations_infer_type_of_template_literal_expressions_as_string (4d52708) with main (51d56d3)

Summary

✅ 28 untouched benchmarks

@MichaelMitchell-at MichaelMitchell-at force-pushed the 07-06-fix_isolated_declarations_infer_type_of_template_literal_expressions_as_string branch 2 times, most recently from bbe7a84 to 139ab90 Compare July 6, 2024 10:13
@Dunqing
Copy link
Member

Dunqing commented Jul 6, 2024

The infer_type_from_expression method can infer TemplateLiteral as a string. But there is a mistake here, when expression exists but can be inferred, we should infer that it is a string type

Expression::TemplateLiteral(lit) => {
if lit.expressions.is_empty() {
Some(self.ast.ts_string_keyword(SPAN))
} else {
None
}
}

Expression::TemplateLiteral(lit) => !lit.expressions.is_empty(),

@MichaelMitchell-at MichaelMitchell-at force-pushed the 07-06-fix_isolated_declarations_infer_type_of_template_literal_expressions_as_string branch from 139ab90 to 8e36a2b Compare July 6, 2024 18:52
@MichaelMitchell-at
Copy link
Contributor Author

@Dunqing I've updated the logic. Please have another look.

@MichaelMitchell-at MichaelMitchell-at force-pushed the 07-06-fix_isolated_declarations_infer_type_of_template_literal_expressions_as_string branch from 8e36a2b to 2c07709 Compare July 6, 2024 18:57
@MichaelMitchell-at MichaelMitchell-at force-pushed the 07-06-fix_isolated_declarations_infer_type_of_template_literal_expressions_as_string branch from 2c07709 to 4d52708 Compare July 6, 2024 20:31
@Boshen Boshen requested a review from Dunqing July 7, 2024 03:00
@Dunqing Dunqing merged commit f8d77e4 into oxc-project:main Jul 7, 2024
25 checks passed
@github-actions github-actions bot mentioned this pull request Jul 8, 2024
Boshen added a commit that referenced this pull request Jul 8, 2024
## [0.17.2] - 2024-07-08

### Features

- 115ac3b allocator: Introduce `FromIn` and `IntoIn` traits. (#4088)
(rzvxa)
- 720983a napi/transform: Allow setting `sourceType` to `transform`
(#4113) (Boshen)
- e386b62 semantic: Check for invalid type import assignments (#4097)
(DonIsaac)

### Bug Fixes

- 5472b7c codegen: 256 indentations level is not enough for codegen
(Boshen)
- 5c31236 isolated-declarations: Keep literal value for readonly
property (#4106) (Dunqing)
- e67c7d1 isolated-declarations: Do not infer type for private
parameters (#4105) (Dunqing)
- 3fcad5e isolated_declarations: Remove nested AssignmentPatterns from
inside parameters (#4077) (michaelm)
- f8d77e4 isolated_declarations: Infer type of template literal
expressions as string (#4068) (michaelm)
- 0f02608 semantic: Bind `TSImportEqualsDeclaration`s (#4100) (Don
Isaac)
- 4413e2d transformer: Missing initializer for readonly consructor
properties (#4103) (Don Isaac)

### Performance

- 7ed27b7 isolated-declarations: Use `FxHashSet` instead of `Vec` to
speed up the `contain` (#4074) (Dunqing)
- 9114c8e semantic: Keep a single map of unresolved references (#4107)
(Luca Bruno)

Co-authored-by: Boshen <Boshen@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-isolated-declarations Isolated Declarations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants