Skip to content

refactor(ast)!: rename is_strict methods to has_use_strict_directive#7783

Merged
graphite-app[bot] merged 1 commit intomainfrom
12-10-refactor_ast_rename_is_strict_methods_to_has_use_strict_directive_
Dec 11, 2024
Merged

refactor(ast)!: rename is_strict methods to has_use_strict_directive#7783
graphite-app[bot] merged 1 commit intomainfrom
12-10-refactor_ast_rename_is_strict_methods_to_has_use_strict_directive_

Conversation

@overlookmotel
Copy link
Member

@overlookmotel overlookmotel commented Dec 10, 2024

The name is_strict was misleading for these methods, because it doesn't tell you if the e.g. function is strict mode, only whether it contains a "use strict" directive. Function::is_strict might return false for a function which does have strict mode semantics because it's e.g. in an ESM file.

Rename these methods to has_use_strict_directive to better reflect what they do.

For Program, change the method to only check for "use strict" directive and not to look at source_type. Semantic should be the source of truth on strict/sloppy mode of AST nodes. It's cheaper to look up the ScopeFlags than to iterate over directives, so don't encourage this anti-pattern by providing a "rival" method.

@graphite-app
Copy link
Contributor

graphite-app bot commented Dec 10, 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-semantic Area - Semantic A-ast Area - AST C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior labels Dec 10, 2024
@overlookmotel overlookmotel marked this pull request as ready for review December 10, 2024 17:06
@codspeed-hq
Copy link

codspeed-hq bot commented Dec 10, 2024

CodSpeed Performance Report

Merging #7783 will not alter performance

Comparing 12-10-refactor_ast_rename_is_strict_methods_to_has_use_strict_directive_ (96a26d3) with main (bde753b)

Summary

✅ 29 untouched benchmarks

@overlookmotel overlookmotel marked this pull request as draft December 10, 2024 18:04
@overlookmotel overlookmotel force-pushed the 12-10-refactor_ast_rename_is_strict_methods_to_has_use_strict_directive_ branch from b7612a3 to b09ef48 Compare December 10, 2024 18:09
@overlookmotel overlookmotel marked this pull request as ready for review December 10, 2024 18:13
@graphite-app graphite-app bot added the 0-merge Merge with Graphite Merge Queue label Dec 10, 2024
@graphite-app
Copy link
Contributor

graphite-app bot commented Dec 10, 2024

Merge activity

…ve` (#7783)

The name `is_strict` was misleading for these methods, because it doesn't tell you if the e.g. function *is* strict mode, only whether it contains a `"use strict"` directive. `Function::is_strict` might return `false` for a function which does have strict mode semantics because it's e.g. in an ESM file.

Rename these methods to `has_use_strict_directive` to better reflect what they do.

For `Program`, change the method to only check for `"use strict"` directive and not to look at `source_type`. `Semantic` should be the source of truth on strict/sloppy mode of AST nodes. It's cheaper to look up the `ScopeFlags` than to iterate over `directives`, so don't encourage this anti-pattern by providing a "rival" method.
@Dunqing Dunqing force-pushed the 12-10-refactor_ast_rename_is_strict_methods_to_has_use_strict_directive_ branch from b09ef48 to 96a26d3 Compare December 10, 2024 23:57
@graphite-app graphite-app bot merged commit 96a26d3 into main Dec 11, 2024
@graphite-app graphite-app bot deleted the 12-10-refactor_ast_rename_is_strict_methods_to_has_use_strict_directive_ branch December 11, 2024 00:03
Boshen added a commit that referenced this pull request Dec 13, 2024
## [0.41.0] - 2024-12-13

- fb325dc ast: [**BREAKING**] `span` field must be the first element
(#7821) (Boshen)

- 96a26d3 ast: [**BREAKING**] Rename `is_strict` methods to
`has_use_strict_directive` (#7783) (overlookmotel)

### Features

- 8991f33 ast: Add `visit_span` to `Visit` and `VisitMut` (#7816)
(overlookmotel)
- f7900ab ast: Add `ArrowFunctionExpression::has_use_strict_directive`
method (#7784) (overlookmotel)
- e727ae9 transformer/class-properties: Transform super member
expressions that are inside static prop initializer (#7815) (Dunqing)

### Bug Fixes

- 7610dc1 parser: Parse `import source from 'mod'` (#7833) (Boshen)
- 9479e2b semantic: Missing references when `export {}` references a
type-only binding and a normal (#7812) (Yunfei He)
- 7a83230 semantic: Missing reference when `export default` references a
type alias binding (#7813) (Dunqing)
- 4a3bca8 semantic: Fix identifying strict mode arrow functions (#7785)
(overlookmotel)
- 5b7e1ad transformer: Remove span of define value (#7811) (Hiroshi
Ogawa)
- 14896cb transformer/class-properties: Create temp vars in correct
scope (#7824) (overlookmotel)
- 25bb6da transformer/class-properties: Fix `ScopeId`s in instance prop
initializers (#7823) (overlookmotel)
- 65b109a transformer/class-properties: No `raw` for generated
`StringLiteral` (#7825) (overlookmotel)
- 2964a61 transformer/class-properties: Unwrap failed when private field
expression doesn't contain optional expression in `ChainExpression`
(#7798) (Dunqing)
- 6fa6785 transformer/class-properties: Panic when the callee or member
is `ParenthesisExpression` or TS-syntax expressions. (#7795) (Dunqing)
- bb22c67 transformer/class-properties: Fix `ScopeId`s in static prop
initializers (#7791) (overlookmotel)
- caa57f1 transformer/class-properties: Fix scope flags in static prop
initializers (#7786) (overlookmotel)

### Performance

- 4448b63 codegen: Faster writing indentation (#7820) (overlookmotel)
- afaaffa codegen: Fast path for `options.print_comments()` (#7806)
(Boshen)

### Refactor

- 0f367e5 semantic: Improve the logic of resolving references to be
cleaner (#7829) (Dunqing)
- 5710950 semantic: Move export-related reference flags logic to visit
functions (#7828) (Dunqing)
- b290ebd transformer: Handle `<CWD>` in test runner (#7799) (Dunqing)
- e70deb9 transformer/class-properties: Locate instance props insertion
location in separate step (#7819) (overlookmotel)
- afc5f1e transformer/class-properties: De-deduplicate code (#7805)
(overlookmotel)
- 47a91d2 transformer/class-properties: Shorten code (#7804)
(overlookmotel)
- 54ef2b9 transformer/class-properties: Rename
`debug_assert_expr_is_not_parenthesis_or_typescript_syntax` (#7803)
(overlookmotel)
- 3cdc47c transformer/class-properties: `#[inline(always)]` on
`assert_expr_neither_parenthesis_nor_typescript_syntax` (#7802)
(overlookmotel)

### Testing

- d72c888 transformer/replace-global-defines: Remove panicking test
(#7838) (overlookmotel)

Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

0-merge Merge with Graphite Merge Queue A-ast Area - AST A-semantic Area - Semantic C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant