Skip to content
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

Strip comments from inlinable text when printing in swiftinterface files #69358

Merged
merged 6 commits into from
Oct 25, 2023

Conversation

harlanhaskins
Copy link
Collaborator

@harlanhaskins harlanhaskins commented Oct 24, 2023

This adds support in InlinableText.cpp for ignoring the ranges of comment tokens and stripping comments from swiftinterface files. Ideally we'd be able to use SwiftSyntax to read in these source ranges and just reprint them while skipping comment trivia, but that infrastructure isn't quite there yet.

Resolves rdar://44318472

@harlanhaskins harlanhaskins changed the title Strip comments from inlinable text [WIP] Strip comments from inlinable text when printing in swiftinterface files Oct 24, 2023
@harlanhaskins
Copy link
Collaborator Author

@swift-ci please smoke test

lib/AST/InlinableText.cpp Outdated Show resolved Hide resolved
lib/AST/InlinableText.cpp Outdated Show resolved Hide resolved
lib/AST/InlinableText.cpp Outdated Show resolved Hide resolved
test/ModuleInterface/if-configs.swift Show resolved Hide resolved
@harlanhaskins harlanhaskins changed the title [WIP] Strip comments from inlinable text when printing in swiftinterface files Strip comments from inlinable text when printing in swiftinterface files Oct 24, 2023
@harlanhaskins harlanhaskins marked this pull request as ready for review October 24, 2023 21:48
@ahoppen ahoppen requested a review from rintaro October 25, 2023 01:01
ahoppen
ahoppen previously approved these changes Oct 25, 2023
Copy link
Contributor

@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.

I’ve got another fun counterexample for you. The following is valid but if you remove the block comment, it becomes invalid.

let x = 1 /*
*/ let b = 2

lib/AST/InlinableText.cpp Outdated Show resolved Hide resolved
lib/AST/InlinableText.cpp Outdated Show resolved Hide resolved
@ahoppen ahoppen dismissed their stale review October 25, 2023 01:09

Accidentally selected approve

@ahoppen
Copy link
Contributor

ahoppen commented Oct 25, 2023

@rintaro Could you also check this, in particular if you have any performance concerns?

Copy link
Contributor

@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.

LGTM

@harlanhaskins
Copy link
Collaborator Author

harlanhaskins commented Oct 25, 2023

For full context, this code:

@inlinable
public func hasComments() -> Bool {
  /* comment! */ // comment!
  #if NOT_PROVIDED
    // comment!
    return true
  #endif

  print(/* 
    comment! 
  */"this should show up")

  print(
    // comment! don't mess up indentation!
    """
    """)

  #if compiler(>=5.3) // comment!
  print(/*comment!*/"")
  #endif

  let x = 1/*
  */let y = 2

  let a = 3
  /* test */let b = 2

  #sourceLocation(file: "if-configs.swift", line: 200)

  #if !NOT_PROVIDED
    // comment!
    return/* comment! */true/* comment! */
  #endif
}

Will be written to a swiftinterface as:

@inlinable public func hasComments() -> Bool {
     

  print(
"this should show up")

  print(
     
    """
    """)

  #if compiler(>=5.3)  
  print( "")
  #endif

  let x = 1
let y = 2

  let a = 3
   let b = 2

  

     
    return true 
}

And bodies without comments pass through unchanged.

@harlanhaskins
Copy link
Collaborator Author

@swift-ci please smoke test

@rintaro
Copy link
Member

rintaro commented Oct 25, 2023

Unrelated to this PR, but I realized current ExtractInactiveRanges doesn't/can't support #if for member expressions or attributes (and more if we expand the feature to others) 😢

@harlanhaskins
Copy link
Collaborator Author

@rintaro Uhhh, that's not great! We didn't support them when this code was originally written so it definitely needs to be updated. That's a secrecy concern.

@rintaro
Copy link
Member

rintaro commented Oct 25, 2023

@harlanhaskins The problem is that AST doesn't have those information. We don't model #if for member expressions in AST. Longer term plan is to migrate it to swift-syntax and SwiftIfConfig But before that, we need to find a workaround somehow.

@harlanhaskins
Copy link
Collaborator Author

I would be totally happy with ExtractInlinableText being fully rewritten in SwiftSyntax. That would be really nice.

@harlanhaskins harlanhaskins merged commit 4a2eefe into apple:main Oct 25, 2023
3 checks passed
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.

None yet

3 participants