Skip to content

Preserve leading comments when converting functions to computed propetries #3236#3238

Open
vishakha1411 wants to merge 3 commits intoswiftlang:mainfrom
vishakha1411:preservecomment
Open

Preserve leading comments when converting functions to computed propetries #3236#3238
vishakha1411 wants to merge 3 commits intoswiftlang:mainfrom
vishakha1411:preservecomment

Conversation

@vishakha1411
Copy link
Contributor

Fixes #3236.

Fixes an issue where documentation comments were dropped when converting zero-parameter functions to computed properties. The refactor now preserves leading comments during conversion. Tested locally. Please let me know if any changes needed or test cases need to be added.

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Could you add a test case for this? Also maybe check that it doesn’t pass without your change. I might just have seen the bug because my build of swift-syntax was too old, now that I look at the code.

@vishakha1411
Copy link
Contributor Author

I have added test cases and checked them without my change as suggested. I ran them both with and without my changes, and they passed in both cases.

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Could you please format your source code using swift-format as described in CONTRIBUTING.md?

)

let bindingSpecifier = syntax.funcKeyword.detached.with(\.tokenKind, .keyword(.var))
let bindingSpecifier = syntax.funcKeyword.detached.with(\.tokenKind, .keyword(.var)).with(\.leadingTrivia, syntax.funcKeyword.leadingTrivia).with(\.trailingTrivia, syntax.funcKeyword.trailingTrivia)
Copy link
Member

Choose a reason for hiding this comment

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

Actually, I would expect just changing the token kind to retain the trivia? Could you double-check if the new tests already pass without your changes, in which case the issue already behaves correctly and I just saw the bug due to an old checkout of swift-syntax?

Copy link
Contributor Author

@vishakha1411 vishakha1411 Jan 20, 2026

Choose a reason for hiding this comment

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

Yes the new tests pass on current main even without my changes so the issue already behaves correctly. I can remove the unnecessary code is preferred.

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.

Converting zero-parameter function to computed property drops comment

2 participants