Skip to content

Fix issue with ast conversion on functions with attributes on arguments. #7761

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

Merged
merged 3 commits into from
Aug 12, 2025

Conversation

cristianoc
Copy link
Collaborator

No description provided.

Now that function attributes and first-argument attributes are split, they need to be re-joined in the old parse tree.
This means that after a ppx, both will be on the first argument.
@cristianoc cristianoc requested a review from nojaf August 11, 2025 14:48
@@ -1 +1 @@
type fn = (~foo: string) => int
type fn = (@as("something") ~foo: string) => int
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This commit illustrates the difference.
This should fix the original issue, but needs testing just in case.

@nojaf
Copy link
Member

nojaf commented Aug 11, 2025

Hmm, I believe the original report was part of a record field, so would that be covered by this? Looking at the mapper I'm not sure, does that always pass that code path?

@cristianoc
Copy link
Collaborator Author

Hmm, I believe the original report was part of a record field, so would that be covered by this? Looking at the mapper I'm not sure, does that always pass that code path?

The record field is after jsx runs. The original code only has an external with a function type.

@nojaf
Copy link
Member

nojaf commented Aug 11, 2025

Right! Got it! Thanks!

@cristianoc
Copy link
Collaborator Author

That said, it would be good to test it with the original example and one ppx.

Copy link

pkg-pr-new bot commented Aug 11, 2025

Open in StackBlitz

rescript

npm i https://pkg.pr.new/rescript-lang/rescript@7761

@rescript/darwin-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-arm64@7761

@rescript/darwin-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-x64@7761

@rescript/linux-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-arm64@7761

@rescript/linux-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-x64@7761

@rescript/win32-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/win32-x64@7761

commit: 856879b

@nojaf
Copy link
Member

nojaf commented Aug 11, 2025

Hello @reebalazs, could you give npm i https://pkg.pr.new/rescript-lang/rescript@7761 a go?

@reebalazs
Copy link

@nojaf @cristianoc Tested it out with my repo: I can confirm that it works. Thank you both!

@cristianoc cristianoc merged commit 8e4d66a into master Aug 12, 2025
27 checks passed
@cristianoc cristianoc deleted the ast-map-fun-with-attribure branch August 12, 2025 06:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants