- Notifications
You must be signed in to change notification settings - Fork471
Preserve @as decorator on record fields when creating interface#7779
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Preserve @as decorator on record fields when creating interface#7779
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull Request Overview
This PR fixes an issue where the@as decorator on record fields was not being preserved when creating interface files. The change ensures that type declarations with attributes are copied verbatim to maintain their decorators.
- Added logic to detect and preserve attributes (like
@as) on type declarations - Enhanced the AttributesUtils module with an
isEmptyfunction - Updated the interface creation process to copy type declarations with attributes verbatim
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| analysis/src/CreateInterface.ml | Core logic to detect attributes and preserve type declarations verbatim |
| tests/analysis_tests/tests/src/CreateInterface.res | Test case with external function and record type using@as decorator |
| tests/analysis_tests/tests/src/expected/CreateInterface.res.txt | Expected output showing preserved@as decorators |
| CHANGELOG.md | Documentation of the bug fix |
Tip: Customize your code reviews with copilot-instructions.md.Create the file orlearn how to get started.
analysis/src/CreateInterface.ml Outdated
| sigItemToString | ||
| (Printtyp.tree_of_type_declaration id typeDecl resStatus) | ||
| in | ||
| Buffer.add_string buf (indent^ newItemStr^"\n")); |
CopilotAIAug 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[nitpick] The conditional logic should be inverted to reduce nesting. Consider using early return pattern:if AttributesUtils.isEmpty attributes then (* normal processing *) else (* verbatim copy *).
analysis/src/CreateInterface.ml Outdated
| (ifnot (AttributesUtils.isEmpty attributes)then | ||
| (* Copy the type declaration verbatim to preserve attributes*) | ||
| Buffer.add_string buf ((lines|>String.concat"\n")^"\n") |
CopilotAIAug 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
String concatenation with^ creates intermediate strings. Consider usingBuffer.add_string for each line separately orString.concat "\n" without the additional concatenation.
| Buffer.add_string buf ((lines|>String.concat"\n")^"\n") | |
| Array.iter (funline ->Buffer.add_string buf line;Buffer.add_char buf'\n') lines |
pkg-pr-newbot commentedAug 21, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
rescript@rescript/darwin-arm64@rescript/darwin-x64@rescript/linux-arm64@rescript/linux-x64@rescript/win32-x64commit: |
analysis/src/CreateInterface.ml Outdated
| let attributes=AttributesUtils.make linesin | ||
| (ifnot (AttributesUtils.isEmpty attributes)then | ||
| (* Copy the type declaration verbatim to preserve attributes*) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Does this work in theelse branch too? Can we always copy verbatim? Or what's the tradeoff here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I've removed theelse branch since every type declaration should be valid in an interface file when copied verbatim 👍 The only tradeoff I can think of is that we'll be copying any non-doc comments within the declaration
9efa7e2 intorescript-lang:masterUh oh!
There was an error while loading.Please reload this page.
Closes#7611