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.

Sign up for GitHub

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

Jump to bottom

refactor(semantic): make SemanticBuilder opaque #4851

Merged
merged 1 commit into from
Aug 14, 2024
Merged

refactor(semantic): make SemanticBuilder opaque #4851

merged 1 commit into from
Aug 14, 2024

Conversation

DonIsaac
Copy link
Contributor

change visibility of building-related methods and properties of SemanticBuilder
from pub to pub(crate).

@github-actions github-actions bot added the A-semantic Area - Semantic label Aug 12, 2024
@DonIsaac DonIsaac mentioned this pull request Aug 12, 2024
Merged
@DonIsaac Graphite App
Copy link
Contributor Author

DonIsaac commented Aug 12, 2024 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @DonIsaac and the rest of your teammates on Graphite Graphite

@DonIsaac DonIsaac marked this pull request as ready for review August 12, 2024 17:25
@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Aug 12, 2024 •
edited
Loading

CodSpeed Performance Report

Merging #4851 will not alter performance

Comparing don/08-12-refactor_semantic_make_semanticbuilder_opaque (ea1e64a) with main (2afccd7)

Summary

✅ 29 untouched benchmarks

@rzvxa
Copy link
Contributor

rzvxa commented Aug 12, 2024 •
edited
Loading

This is lovely, It's so annoying having builder fields available from the outside world. But let's not merge it until we get our other breaking changes ready as well.
Adding AstNodeId and reordering call expression fields are also breaking changes so it would be worth it to wait just so we can increase the minor version once for all of these.

@Boshen Boshen changed the base branch from don/08-12-docs_semantic_improve_documentation to graphite-base/4851 August 13, 2024 02:14
@Boshen Boshen force-pushed the don/08-12-refactor_semantic_make_semanticbuilder_opaque branch from dd1f9d8 to 439e992 Compare August 13, 2024 02:17
@Boshen Boshen force-pushed the graphite-base/4851 branch from 3eda94e to 0a01a47 Compare August 13, 2024 02:17
@Boshen Boshen changed the base branch from graphite-base/4851 to main August 13, 2024 02:18
@DonIsaac DonIsaac requested a review from Boshen August 13, 2024 03:47
@Boshen
Copy link
Member

Boshen commented Aug 13, 2024

Let me test Rolldown before merging.

@rzvxa rzvxa mentioned this pull request Aug 13, 2024
Merged
@DonIsaac DonIsaac force-pushed the don/08-12-refactor_semantic_make_semanticbuilder_opaque branch from 439e992 to 841d6bd Compare August 13, 2024 19:40
@overlookmotel
Copy link
Contributor

Very much agree with this change. If Rolldown is relying on accessing properties/methods of SemanticBuilder, it shouldn't be, and we should provide alternative methods on Semantic for them to access the info they need.

@Boshen Boshen changed the title refactor(semantic)!: make SemanticBuilder opaque refactor(semantic): make SemanticBuilder opaque Aug 14, 2024
@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label Aug 14, 2024
@graphite-app Graphite App
Copy link

graphite-app bot commented Aug 14, 2024 •
edited
Loading

Merge activity

  • Aug 14, 4:14 AM EDT: The merge label 'merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Aug 14, 4:14 AM EDT: Boshen added this pull request to the Graphite merge queue.
  • Aug 14, 4:19 AM EDT: Boshen merged this pull request with the Graphite merge queue.

@DonIsaac
refactor(semantic): make SemanticBuilder opaque (#4851)
ea1e64a 
change visibility of building-related methods and properties of SemanticBuilder
from `pub` to `pub(crate)`.
@Boshen Boshen force-pushed the don/08-12-refactor_semantic_make_semanticbuilder_opaque branch from 841d6bd to ea1e64a Compare August 14, 2024 08:15
@graphite-app graphite-app bot merged commit ea1e64a into main Aug 14, 2024
24 checks passed
@graphite-app graphite-app bot deleted the don/08-12-refactor_semantic_make_semanticbuilder_opaque branch August 14, 2024 08:19
@oxc-bot oxc-bot mentioned this pull request Aug 18, 2024
Merged
Boshen added a commit that referenced this pull request Aug 18, 2024
@oxc-bot @Boshen
Release crates v0.24.3 (#4950)
5d0fb97 
## [0.24.3] - 2024-08-18

### Features

- d49fb16 oxc_codegen: Support generate range leading comments (#4898)
(IWANABETHATGUY)
- 80d0d1f semantic: Check for invalid interface heritage clauses (#4928)
(DonIsaac)
- 48821c0 semantic,syntax: Add SymbolFlags::ArrowFunction (#4946)
(DonIsaac)
- f1fcdde transformer: Support react fast refresh (#4587) (Dunqing)
- 0d79122 transformer: Support logical-assignment-operators plugin
(#4890) (Dunqing)
- ab1d08c transformer: Support `optional-catch-binding` plugin (#4885)
(Dunqing)
- 69da9fd transformer: Support nullish-coalescing-operator plugin
(#4884) (Dunqing)
- 3a66e58 transformer: Support exponentiation operator plugin (#4876)
(Dunqing)
- f88cbcd transformer: Add `BoundIdentifier::new_uid_in_current_scope`
method (#4903) (overlookmotel)
- 1e6d0fe transformer: Add methods to `BoundIdentifier` (#4897)
(overlookmotel)
- fd34640 traverse: Support `generate_uid_based_on_node` method in
`TraverseCtx` (#4940) (Dunqing)
- 72a37fc traverse: Support `clone_identifier_reference` method in
`TraverseCtx` (#4880) (Dunqing)

### Bug Fixes

- c0b26f4 ast: Do not include `scope_id` fields in JSON AST (#4858)
(overlookmotel)
- bbf9ec0 codegen: Add missing `declare` to `PropertyDefinition` (#4937)
(Boshen)
- f210cf7 codegen: Print `TSSatisfiesExpression` and
`TSInstantiationExpression` (#4936) (Boshen)
- 21f5762 codegen: Minify large numbers (#4889) (Boshen)
- e8de4bd codegen: Fix whitespace issue when minifying `x in new
Error()` (#4886) (Boshen)
- a226962 codegen: Print `TSNonNullExpression` (#4869) (Boshen)
- 3da33d3 codegen: Missing parenthesis for `PrivateInExpression` (#4865)
(Boshen)
- 1808529 codegen: Dedupe pure annotation comments (#4862)
(IWANABETHATGUY)
- d3bbc62 isolated-declarations: Declare modifier of PropertyDefinition
should not be retained (#4941) (Dunqing)
- 8e80f59 isolated_declarations: Class properties should still be lifted
from private constructors (#4934) (michaelm)
- b3ec9e5 isolated_declarations: Always emit module declarations that
perform augmentation (#4919) (michaelm)
- 0fb0b71 isolated_declarations: Always emit module declarations (#4911)
(michaelm)
- 4a16916 isolated_declarations: Support expando functions (#4910)
(michaelm)
- 508644a linter/tree-shaking: Correct the calculation of `>>`, `<<` and
`>>>` (#4932) (mysteryven)
- 46cb1c1 minifier: Handle `Object.definedPropert(exports` for
@babel/types/lib/index.js (#4933) (Boshen)
- 81fd637 minifier: Do not fold `0 && (module.exports = {})` for
`cjs-module-lexer` (#4878) (Boshen)
- 879a271 minifier: Do not join `require` calls for `cjs-module-lexer`
(#4875) (Boshen)
- 1bdde2c parser: Detect @flow in `/** @flow */ comment (#4861) (Boshen)
- 2476dce transformer: Remove an `ast.copy` from
`NullishCoalescingOperator` transform (#4913) (overlookmotel)
- 248a757 transformer/typescript: Typescript syntax within
`SimpleAssignmentTarget` with `MemberExpressions` is not stripped
(#4920) (Dunqing)

### Documentation

- 47c9552 ast, ast_macros, ast_tools: Better documentation for `Ast`
helper attributes. (#4856) (rzvxa)
- 0a01a47 semantic: Improve documentation (#4850) (DonIsaac)
- 9c700ed transformer: Add README including style guide (#4899)
(overlookmotel)

### Refactor

- a6967b3 allocator: Correct code comment (#4904) (overlookmotel)
- 90d0b2b allocator, ast, span, ast_tools: Use `allocator` as var name
for `Allocator` (#4900) (overlookmotel)
- 1eb59d2 ast, isolated_declarations, transformer: Mark
`AstBuilder::copy` as an unsafe function (#4907) (overlookmotel)
- 8e8fcd0 ast_tools: Rename `oxc_ast_codegen` to `oxc_ast_tools`.
(#4846) (rzvxa)
- 786bf07 index: Shorten code and correct comment (#4905)
(overlookmotel)
- ea1e64a semantic: Make SemanticBuilder opaque (#4851) (DonIsaac)
- 5fd1701 sourcemap: Lower the `msrv`. (#4873) (rzvxa)
- 48a1c32 syntax: Inline trivial bitflags methods (#4877)
(overlookmotel)
- 452187a transformer: Rename `BoundIdentifier::new_uid_in_root_scope`
(#4902) (overlookmotel)
- 707a01f transformer: Re-order `BoundIdentifier` methods (#4896)
(overlookmotel)
- 117dff2 transformer: Improve comments for `BoundIdentifier` helper
(#4895) (overlookmotel)

Co-authored-by: Boshen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rzvxa rzvxa rzvxa approved these changes

@camc314 camc314 camc314 approved these changes

@Boshen Boshen Awaiting requested review from Boshen Boshen is a code owner

Assignees
No one assigned
Labels
0-merge Merge with Graphite Merge Queue A-semantic Area - Semantic
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@DonIsaac @rzvxa @Boshen @overlookmotel @camc314