search

google / swift

The Swift Programming Language
https://swift.org/
840 stars 66 forks source link

Fix a bug where code lines are lost for OrderedImports rule #245

Closed adellibovi closed 5 years ago

adellibovi commented 5 years ago

Hi,

This fix an issue where code blocks are lost during the processing. This happens when both NoAccessLevelOnExtensionDeclaration and OrderedImports are active. It basically happens for every declaration (struct, class etc..) followed by any extension declaration that need an update of the modified.

For example, the following code trigger the issue:

struct Shadow { }
internal extension ShadowExt {}
adellibovi commented 5 years ago

Sorry, also noticed that probably (I didn’t try it yet) this could be fixed via a similar approach of: https://github.com/google/swift/commit/d97dd1c29a8f51729327fdb2c3603eac39b7579d I.e.: keeping the leading when changing the modifier of the extension. What’s the preferred way?

dabelknap commented 5 years ago

Thanks for the PR. As you pointed out, another way to correct the behavior would be akin to d97dd1c. Basically, when the internal keyword gets removed, its leading trivia isn't being transferred to extension. This gets rid of any .newlines which is what causes OrderedImports to lose code. The trivia problem in NoAccessLevelOnExtensionDeclaration definitely needs to be fixed at some point. The changes you made to OrderedImports are still useful and worth keeping since it makes the rule more robust against these types of errors. You're welcome to make the corrections to NoAccessLevelOnExtensionDeclaration if you'd like, or we can merge your PR as-is and deal with it in a later PR.

adellibovi commented 5 years ago

hi @dabelknap thanks for the quick feedback, I was checking the implementation of NoAccessLevelOnExtensionDeclaration and it looks like that is correct: leadingTrivia is transferred to extension (this is currently tested via testPreservesCommentOnRemovedModifier), but what happen is that we have now an empty modifier, which now returns an empty leadingTrivia (I am not sure if this is the expected behavior of SwiftSyntax). Do you have any ideas? Otherwise I would agree on merging this as-is, thanks :)

dabelknap commented 5 years ago

Hmm...it must be a weird edge case or something. I was definitely able to produce behavior where the leading trivia vanished. I'll go ahead and merge these changes and debug NoAccessLevelOnExtensionDeclaration separately.