Revert "Implement ClangImporter importSourceLoc and importSourceRange"

[zoecarver]

Reverts #40010.

Verifying that this broke the source compatibility test suite.

[zoecarver]

CC @NuriAmari

[zoecarver]

Please don't merge until we verify that this was actually the commit that broke the source compatibility test suite.

[zoecarver]

@swift-ci please test source compatibility

[slavapestov]

Thanks @zoecarver!

[drodriguez]

Slightly early to call, but seems that ProcedureKit and SwiftyStoreKit are passing in those two builds of the compatibility suite. Seems that both are related to NSError. I don't see the relation clearly with Nuri's changes, but I think reverting at this point is not a bad idea.

[drodriguez]

Found the work around, at least for SwiftyStoreKit (ProcedureKit is a little bit more complicated to setup, but I think if it is not because of the same, it might be something related).

If after https://github.com/apple/swift/blob/7afb4752791c3ca00a7e7de24716ec51c21e82ad/lib/ClangImporter/ImportDecl.cpp#L2926 one adds a line errorWrapper->setBraces(loc); (sic) the assert is not hit. StructDecl::getSourceRange() uses the start of the struct, and the end of the braces. But the braces are not initialized in the constructor. Adding that loc (which is the same as the start of the struct) seems to bypass the problem. I imagine that before the changes, both were empty SourceLoc which didn't trigger the assert.

[bnbarham]

Found the work around, at least for SwiftyStoreKit (ProcedureKit is a little bit more complicated to setup, but I think if it is not because of the same, it might be something related).

If after

https://github.com/apple/swift/blob/7afb4752791c3ca00a7e7de24716ec51c21e82ad/lib/ClangImporter/ImportDecl.cpp#L2926

one adds a line errorWrapper->setBraces(loc); (sic) the assert is not hit. StructDecl::getSourceRange() uses the start of the struct, and the end of the braces. But the braces are not initialized in the constructor. Adding that loc (which is the same as the start of the struct) seems to bypass the problem. I imagine that before the changes, both were empty SourceLoc which didn't trigger the assert.

(Enum|Protocol|Class|Struct)Decl all have brace locations where getRange is made up from the <Type>Loc and braces end loc. I handled this in the PR I mentioned (https://github.com/apple/swift/pull/39701/files#diff-d87e42f4d1c72f623a9f9246f244b1ad6f375fa7d1dce3d162ebb302d17f8922) by just checking the braces loc since it seemed a bit weird to set braces to something that isn't braces. Could probably make the argument for using the @whatever and @end ranges though.

[zoecarver]

@NuriAmari and @drodriguez do you want to fix this or should I revert?

[drodriguez]

@zoecarver revert with this one. We will resend the other one with whatever fix is needed and test it properly.

[zoecarver]

Sounds good. Thanks, Daniel!

@swift-ci please smoke test

[zoecarver]

@swift-ci please smoke test Linux

[zoecarver]

@swift-ci please smoke test Windows

[zoecarver]

@swift-ci please test Windows

[beccadax]

@NuriAmari I didn't see the original PR, but I'd like to discuss whether we want the original change at all. (This is an awkward place to discuss it, so if you'd rather do that elsewhere, let me know.)

I'm not convinced that it's a good idea to put translated Clang source locations into AST nodes for three reasons:

1. SourceLocs get used for more than just diagnostics; they are also used for things like availability scoping and occasionally even deciding which declarations are synthesized and don't need to be checked. It's difficult to know what you'll be affecting with this.

2. We generally assume that a SourceLoc is at the start of a Swift token so that the Swift lexer can lex from there to the end of the token. C lexes similarly enough that we can often get away with translating source locations, but not always. The more we translate, the more we risk hitting one of the exceptions.

3. When you use the variant of diagnose() that takes a Decl instead of a SourceLoc or DeclNameLoc, Swift will produce a generated interface for the decl and diagnose the error in that interface. For diagnostics that are not specific to clang interop, this is arguably a better user experience because it shows you how Swift imported the declaration, which may be part of the error's cause. When you import a SourceLoc for the decl, Swift will instead diagnose these errors in the C header, which means you have to know Swift's importing rules by heart.

To be sure, in the cases where 3 is involved, it would be helpful to also see the Clang declaration. Perhaps the diagnostic engine could arrange to have an additional note emitted pointing to the imported declaration in the header file. But I don't think we should only emit at that source location.

(This could also avoid eagerly importing source buffers we won't ever use just so that we can create Swift SourceLocs for them.)

[zoecarver]

@swift-ci please smoke test linux.

[NuriAmari]

@beccadax Thanks for bringing these points to my attention, I wasn't aware of the first two. With this new information, I don't feel strongly that the original change is necessarily desirable. I simply needed a helper to translate Clang source locations for use in diagnostics and figured I would implement these methods rather than create a new one. I do have some questions / thoughts about the matter though.

1. SourceLocs get used for more than just diagnostics; they are also used for things like availability scoping and occasionally even deciding which declarations are synthesized and don't need to be checked. It's difficult to know what you'll be affecting with this.

2. We generally assume that a SourceLoc is at the start of a Swift token so that the Swift lexer can lex from there to the end of the token. C lexes similarly enough that we can often get away with translating source locations, but not always. The more we translate, the more we risk hitting one of the exceptions.

I suppose my question is, it is somehow desirable or correct to provide empty SourceLocs, or is it more that providing valid source locations is a risky change, but one that would be an improvement if done correctly? If empty source locations are desired, perhaps it would make sense to replace some (if not all) calls to importSourceLoc with explicit empty SourceLocs created inline. The way it is now, it looks like callers expect the function to be implemented, but no one got around to it. It looks like I'm not the first to try this and I imagine, without a change (even just a comment), I might not be the last.

3. When you use the variant of diagnose() that takes a Decl instead of a SourceLoc or DeclNameLoc, Swift will produce a generated interface for the decl and diagnose the error in that interface. For diagnostics that are not specific to clang interop, this is arguably a better user experience because it shows you how Swift imported the declaration, which may be part of the error's cause. When you import a SourceLoc for the decl, Swift will instead diagnose these errors in the C header, which means you have to know Swift's importing rules by heart.

Fair enough, my use case is entirely for producing diagnostics where which the ClangImporter fails, so there is no Swift Decl to speak of. I still think a sort of utility method for converting source locations would be helpful, lest the few lines required be repeated for every diagnostics. In another PR I've called this method convertSourceLocation and I would like to introduce such a method in some form. Let me know if you have any thoughts on this, perhaps a comment explaining why we have two methods that seemingly do the same thing would be appropriate.

[beccadax]

I actually recently wrote a helper for diagnosing errors in headers, although it's not yet merged (or even code-reviewed): b372ff6

This is designed only for use in ClangImporter, and only for diagnostics we want to emit in headers, but it might be something we could extend further.