Skip to content

fix(ast): estree compat Function#8972

Merged
graphite-app[bot] merged 1 commit intomainfrom
fix-ast-estree-function
Feb 9, 2025
Merged

fix(ast): estree compat Function#8972
graphite-app[bot] merged 1 commit intomainfrom
fix-ast-estree-function

Conversation

@hi-ogawa
Copy link
Contributor

@hi-ogawa hi-ogawa commented Feb 8, 2025

Part of #2854

diff of estree_test262.snap

-Positive Passed: 6233/44293 (14.07%)
+Positive Passed: 13042/44293 (29.44%)

@github-actions github-actions bot added the A-ast Area - AST label Feb 8, 2025
Copy link
Contributor Author

hi-ogawa commented Feb 8, 2025


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

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

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

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

@hi-ogawa hi-ogawa changed the title fix: estree compat FunctionBody fix(ast): estree compat FunctionBody Feb 8, 2025
@github-actions github-actions bot added the C-bug Category - Bug label Feb 8, 2025
@hi-ogawa hi-ogawa changed the title fix(ast): estree compat FunctionBody fix(ast): estree compat Function Feb 8, 2025
@codspeed-hq
Copy link

codspeed-hq bot commented Feb 8, 2025

CodSpeed Performance Report

Merging #8972 will not alter performance

Comparing fix-ast-estree-function (a2883b1) with main (f2d28f3)

Summary

✅ 33 untouched benchmarks

@hi-ogawa hi-ogawa changed the base branch from main to graphite-base/8972 February 8, 2025 06:53
@hi-ogawa hi-ogawa changed the base branch from graphite-base/8972 to main February 8, 2025 06:53
@hi-ogawa hi-ogawa force-pushed the fix-ast-estree-function branch from af3bf21 to a7e07fa Compare February 8, 2025 06:54
@hi-ogawa hi-ogawa changed the base branch from main to 02-08-test_ast_remove_obsolete_ast_snapshot_tests February 8, 2025 06:54
@hi-ogawa hi-ogawa changed the base branch from 02-08-test_ast_remove_obsolete_ast_snapshot_tests to graphite-base/8972 February 9, 2025 02:42
@hi-ogawa hi-ogawa force-pushed the fix-ast-estree-function branch from d9257fe to e5ab419 Compare February 9, 2025 02:50
@hi-ogawa hi-ogawa marked this pull request as ready for review February 9, 2025 02:56
Comment on lines 1705 to 1707
pub directives: Vec<'a, Directive<'a>>,
#[estree(rename = "body")]
pub statements: Vec<'a, Statement<'a>>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

both directives and statements should be in body, but append_to macro seems to break for non optional field. I'll look into updating macro for this case separately.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, extending #[estree(append_to = ...)] to cover appending Vecs too would be ideal. We could alternatively add #[estree(prepend_to = ...)], which would make more sense here. But I think for now we already have enough attributes - probably better to use append_to at least for now.

Let me know if you have trouble working out how to adapt the codegen. I think it mostly makes sense, but I wrote it, so of course it makes sense to me!

@Boshen Boshen requested a review from overlookmotel February 9, 2025 04:14
@Boshen Boshen force-pushed the fix-ast-estree-function branch from e5ab419 to 161ebd1 Compare February 9, 2025 04:19
@Boshen Boshen force-pushed the graphite-base/8972 branch from 840c440 to 4803059 Compare February 9, 2025 04:19
@Boshen Boshen changed the base branch from graphite-base/8972 to main February 9, 2025 04:20
@Boshen Boshen force-pushed the fix-ast-estree-function branch from 161ebd1 to 2315309 Compare February 9, 2025 04:20
Copy link
Member

@overlookmotel overlookmotel left a comment

Choose a reason for hiding this comment

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

Great!

Side note: I'm wondering in what circumstances the expression field is true?

Seems not to be true for function expressions (AST explorer). So when is it true? I guess we'll find out as we progress through the test cases...

@overlookmotel overlookmotel added the 0-merge Merge with Graphite Merge Queue label Feb 9, 2025
Copy link
Member

overlookmotel commented Feb 9, 2025

Merge activity

  • Feb 9, 1:52 PM EST: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Feb 9, 1:52 PM EST: A user added this pull request to the Graphite merge queue.
  • Feb 9, 1:57 PM EST: A user merged this pull request with the Graphite merge queue.

Part of #2854

diff of estree_test262.snap

```diff
-Positive Passed: 6233/44293 (14.07%)
+Positive Passed: 13042/44293 (29.44%)
```
@graphite-app graphite-app bot force-pushed the fix-ast-estree-function branch from 2315309 to a2883b1 Compare February 9, 2025 18:53
@graphite-app graphite-app bot merged commit a2883b1 into main Feb 9, 2025
25 checks passed
@graphite-app graphite-app bot deleted the fix-ast-estree-function branch February 9, 2025 18:57
@hi-ogawa
Copy link
Contributor Author

hi-ogawa commented Feb 9, 2025

Side note: I'm wondering in what circumstances the expression field is true?

I just looked into estree and this is probably for some ancient deprecated feature https://github.com/estree/estree/blob/2b48e56efc223ea477a45b5e034039934c5791fa/deprecated.md#functions

In https://bugzilla.mozilla.org/show_bug.cgi?id=1083458, I found a weird syntax like function(l) null for the first time 😮

Dunqing pushed a commit that referenced this pull request Feb 10, 2025
Part of #2854

diff of estree_test262.snap

```diff
-Positive Passed: 6233/44293 (14.07%)
+Positive Passed: 13042/44293 (29.44%)
```
@overlookmotel
Copy link
Member

I just looked into estree and this is probably for some ancient deprecated feature https://github.com/estree/estree/blob/2b48e56efc223ea477a45b5e034039934c5791fa/deprecated.md#functions

In https://bugzilla.mozilla.org/show_bug.cgi?id=1083458, I found a weird syntax like function(l) null for the first time 😮

Ah ha! function(l) null - thank god this did not take off!

graphite-app bot pushed a commit that referenced this pull request Feb 10, 2025
Part of #2854
Related: #8972 (comment)

I updated `append_to` macro to support concatenating two `Vec`. The logic is very similar so I reused it, but maybe it's better to introduce a new macro or maybe rename `append_to`?
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 C-bug Category - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants