[SourceKit] Support location info for macro-expanded Clang imports #82006

j-hui · 2025-06-05T01:54:23Z

Currently, when we jump-to-definition for decls that are macro-expanded
from Clang imported decls (e.g., safe overloads generated by
@_SwiftifyImport), setLocationInfo() emits a bongus location pointing to
a generated buffer, leading the IDE to try to jump to a file that does
not exist.

The root cause here is that setLocationInfo() calls getOriginalRange()
(earlier, getOriginalLocation()), which was not written to account for
such cases where a macro is generated from another generated buffer
whose kind is AttributeFromClang.

This patch fixes setLocationInfo() with some refactoring:

getOriginalRange() is inlined into setLocationInfo(), so that the
generated buffer-handling logic is localized to that function. This
includes how it handles buffers generated for ReplacedFunctionBody.
getOriginalLocation() is used in a couple of other places that only
care about macros expanded from the same buffer (so other generated
buffers not not relevant). This "macro-chasing" logic is simplified
and moved from ModuleDecl::getOriginalRange() to a free-standing
function, getMacroUnexpandedRange() (there is no reason for it to be
a method of ModuleDecl).
GeneratedSourceInfo now carries an extra ClangNode field, which is
populated by getClangSwiftAttrSourceFile() when constructing
a generated buffer for an AttributeFromClang. This could probably
be union'ed with one or more of the other fields in the future.

rdar://151020332

j-hui · 2025-06-05T01:54:31Z

@swift-ci please test

test/SourceKit/Swiftify/clang-overload-cursor-info.swift

tools/SourceKit/lib/SwiftLang/SwiftSourceDocInfo.cpp

lib/ClangImporter/ImportDecl.cpp

hamishknight · 2025-06-06T09:46:28Z

getOriginalLocation() is used in a couple of other places that don't
actually care about the original location

I don't think that's true, both getExternalRawLocsForDecl and getMappedLocation use the returned location

Xazax-hun · 2025-06-06T13:01:53Z

lib/Basic/SourceLoc.cpp

+    case GeneratedSourceInfo::PrettyPrinted:
+    case GeneratedSourceInfo::DefaultArgument:
+    case GeneratedSourceInfo::AttributeFromClang:
+      return SourceRange();


Is this the behavior that we want? What happens when there is a generated default argument inside the macro generated buffer?

This is the current behavior, which I'm trying to preserve.

That said, I'm not sure how we should handle the case you bring up; @bnbarham do you have thoughts on this?

bnbarham · 2025-06-06T16:24:14Z

I don't think that's true, both getExternalRawLocsForDecl and getMappedLocation use the returned location

I believe the full statement is true, ie.

only the outermost buffer a macro is expanded from

At least in the cases we have today (the only original location we care about is the file that the macro was expanded from)

EDIT: Ah, I see, you were referring to the fact that neither getExternalRawLocsForDecl or getMappedLocation were using the returned location 👍

bnbarham · 2025-06-06T17:46:49Z

lib/AST/Module.cpp

-    std::tie(UnderlyingBufferID, MainLoc) =
-        D->getModuleContext()->getOriginalLocation(MainLoc);
-    if (BufferID != UnderlyingBufferID)
+    auto R = SM.getUnexpandedMacroRange(MainLoc);


I'm fine with the less generic name, but it's unfortunate we have to do another findBufferContainingLoc when we would already have done it in getUnexpandedMacroRange (it's not the cheapest operation ever, though IIRC we do cache the last lookup so... maybe it's fine). Is there any reason not to return the buffer id as well?

But also, this still needs to assign MainLoc if we get a range back

Yeah, I figured this should be ok because it's cached:

swift/lib/Basic/SourceLoc.cpp

Lines 521 to 525 in 17ce04f

// Check the last buffer we looked in.

if (auto lastBufferID = LocCache.lastBufferID) {

if (isInBuffer(*lastBufferID))

return *lastBufferID;

}

I'm happy to revert to returning the tuple if you don't want to rely on this.

bnbarham · 2025-06-06T17:52:16Z

tools/SourceKit/lib/SwiftLang/SwiftSourceDocInfo.cpp

+    // restore them if we encounter generated buffer kinds we don't care about.
+    auto VDRange = range;
+    auto VDBufferID = bufferID;
+    while (auto *info = SM.getGeneratedSourceInfo(bufferID)) {


Probably worth splitting this one out into its own function here as well IMO

I ended up refactoring the Location-setting code into a helper (and also simplifying the loop's control flow) so that the generated buffer-chasing logic becomes the focus of setLocationInfo().

j-hui · 2025-06-06T18:10:10Z

I don't think that's true, both getExternalRawLocsForDecl and getMappedLocation use the returned location

You're right, I somehow missed this entirely 🤦

j-hui · 2025-06-06T19:09:47Z

@swift-ci please test

j-hui · 2025-06-06T20:34:25Z

@swift-ci please test

j-hui · 2025-06-10T17:07:47Z

@swift-ci please test

j-hui · 2025-06-10T20:02:47Z

@swift-ci please test

j-hui · 2025-06-11T02:13:31Z

@swift-ci please test

hnrklssn

LGTM

j-hui · 2025-06-11T15:54:11Z

@swift-ci please test macos platform

bnbarham · 2025-06-12T22:04:34Z

tools/SourceKit/lib/SwiftLang/SwiftSourceDocInfo.cpp

+static void setLocationInfoForRange(SourceManager &SM, SourceRange R,
+                                    unsigned BufID,
+                                    LocationInfo &Location,
+                                    bool presumed = false) {


There's here and MappedDecl where the parameter name switches casing, would be nice to have them be consistent one way or the other

I fixed this up (haven't pushed yet), but the MappedDecl parameter already lives alongside mixed case names: https://github.com/swiftlang/swift/pull/82006/files#diff-d29df7eb941cda5dc4dc36df1f3220fe4468a9b58d4fa8ca251c76337f353589L8672

So maybe I should leave that as is?

I fixed this up (haven't pushed yet), but the MappedDecl parameter already lives alongside mixed case names

It was module and MappedDecl is new right? I'd just be locally consistent if nothing else 😅 (so mappedDecl).

Oh, right. But the MappedDecl parameter in getClangSwiftAttrSourceFile() comes from the MappedDecl parameter in importNontrivialAttribute(), which already has it upper-cased. I was aiming to keep the variable name consistent across the function call boundary..

bnbarham · 2025-06-12T22:30:38Z

tools/SourceKit/lib/SwiftLang/SwiftSourceDocInfo.cpp

+          // through other macro expansions. Fall back to setting location based
+          // on VD's original source range because mapping location back to the
+          // original file might be tricky.
+          setLocationInfoForRange(SM, VDRange, VDBufID, Location);


What did we do here previously 🤔?

The previous behavior is buried in getOriginalRange(), which is to just bail out and use the original SourceRange:

swift/lib/AST/Module.cpp

Lines 879 to 886 in 00aa22f

case GeneratedSourceInfo::ReplacedFunctionBody:

// There's not really any "original" location for locations within

// replaced function bodies. The body is actually different code to the

// original file.

case GeneratedSourceInfo::PrettyPrinted:

case GeneratedSourceInfo::AttributeFromClang:

// No original location, return the original buffer/location

return {startBufferID, startRange};

Now that you mention this, I'm not sure if that is the correct behavior, but I wasn't sure whether this scenario is even reachable (ReplacedFunctionBody would mostly be for source edits, right? So they shouldn't be embedded within other generated macro buffers?)

Yeah, they shouldn't ever be embedded within a macro. TBH I would prefer checking for ReplacedFunctionBody specifically and re-using getUnexpanded... for the macro portion (which would also eliminate the duplicate code there).

Re-using getUnexpanded...() will need special handling for the AttributeFromClang case though, which is unnecessary for the two other call sites. I could push the special case logic in there, but my thinking was that by inlining getUnexpandedMacroRange() here I could keep the complexity close to where the slightly different behavior is needed.

Ah... :( Okay

tools/SourceKit/lib/SwiftLang/SwiftSourceDocInfo.cpp

Currently, when we jump-to-definition for decls that are macro-expanded from Clang imported decls (e.g., safe overloads generated by @_SwiftifyImport), setLocationInfo() emits a bongus location pointing to a generated buffer, leading the IDE to try to jump to a file that does not exist. The root cause here is that setLocationInfo() calls getOriginalRange() (earlier, getOriginalLocation()), which was not written to account for such cases where a macro is generated from another generated buffer whose kind is 'AttributeFromClang'. This patch fixes setLocationInfo() with some refactoring: - getOriginalRange() is inlined into setLocationInfo(), so that the generated buffer-handling logic is localized to that function. This includes how it handles buffers generated for ReplacedFunctionBody. - getOriginalLocation() is used in a couple of other places that only care about macros expanded from the same buffer (so other generated buffers not not relevant). This "macro-chasing" logic is simplified and moved from ModuleDecl::getOriginalRange() to a free-standing function, getMacroUnexpandedRange() (there is no reason for it to be a method of ModuleDecl). - GeneratedSourceInfo now carries an extra ClangNode field, which is populated by getClangSwiftAttrSourceFile() when constructing a generated buffer for an 'AttributeFromClang'. This could probably be union'ed with one or more of the other fields in the future. rdar://151020332

j-hui · 2025-06-13T01:24:48Z

@swift-ci please test

j-hui · 2025-06-13T04:55:29Z

@swift-ci please test Linux platform

j-hui · 2025-06-13T18:21:48Z

@swift-ci please test

j-hui · 2025-06-14T06:42:14Z

@swift-ci please test

j-hui · 2025-06-16T11:58:38Z

@swift-ci please test

hamishknight

Thanks!

tools/SourceKit/lib/SwiftLang/SwiftSourceDocInfo.cpp

j-hui · 2025-06-18T09:26:45Z

@swift-ci please test

j-hui · 2025-06-18T23:49:31Z

@swift-ci please test

j-hui requested review from bnbarham, hnrklssn and hamishknight June 5, 2025 01:54

j-hui requested review from rintaro, zoecarver, egorzhdan, beccadax, ian-twilightcoder, Xazax-hun, fahadnayyar, susmonteiro, DougGregor, hborla, slavapestov and xedin as code owners June 5, 2025 01:54

hnrklssn reviewed Jun 5, 2025

View reviewed changes

test/SourceKit/Swiftify/clang-overload-cursor-info.swift Outdated Show resolved Hide resolved

hnrklssn reviewed Jun 5, 2025

View reviewed changes

tools/SourceKit/lib/SwiftLang/SwiftSourceDocInfo.cpp Outdated Show resolved Hide resolved

hnrklssn reviewed Jun 5, 2025

View reviewed changes

lib/ClangImporter/ImportDecl.cpp Show resolved Hide resolved

j-hui force-pushed the jump-to-def-for-macro-expanded-clang-imports branch from ccc5e18 to 17ce04f Compare June 6, 2025 00:36

Xazax-hun reviewed Jun 6, 2025

View reviewed changes

bnbarham reviewed Jun 6, 2025

View reviewed changes

j-hui force-pushed the jump-to-def-for-macro-expanded-clang-imports branch from 17ce04f to 3e5dbf0 Compare June 6, 2025 19:04

j-hui force-pushed the jump-to-def-for-macro-expanded-clang-imports branch from 3e5dbf0 to 3cb05d3 Compare June 6, 2025 20:34

j-hui force-pushed the jump-to-def-for-macro-expanded-clang-imports branch from 3cb05d3 to aa6a39c Compare June 10, 2025 17:06

j-hui force-pushed the jump-to-def-for-macro-expanded-clang-imports branch from aa6a39c to 637c4bf Compare June 10, 2025 19:57

j-hui force-pushed the jump-to-def-for-macro-expanded-clang-imports branch from 637c4bf to 33b80ba Compare June 11, 2025 02:09

j-hui requested review from bnbarham, hnrklssn and Xazax-hun June 11, 2025 02:16

j-hui mentioned this pull request Jun 11, 2025

🍒 [6.2] [SourceKit] Support location info for macro-expanded Clang imports #82171

Merged

hnrklssn approved these changes Jun 11, 2025

View reviewed changes

bnbarham reviewed Jun 12, 2025

View reviewed changes

j-hui force-pushed the jump-to-def-for-macro-expanded-clang-imports branch from 33b80ba to 44aba13 Compare June 13, 2025 01:22

bnbarham approved these changes Jun 13, 2025

View reviewed changes

j-hui enabled auto-merge June 13, 2025 01:39

hamishknight approved these changes Jun 16, 2025

View reviewed changes

tools/SourceKit/lib/SwiftLang/SwiftSourceDocInfo.cpp Outdated Show resolved Hide resolved

[NFC] Refactor setLocationInfo to return LocationInfo instead

6b36996

j-hui merged commit a0d3ad7 into swiftlang:main Jun 19, 2025
3 of 5 checks passed

	// Check the last buffer we looked in.
	if (auto lastBufferID = LocCache.lastBufferID) {
	if (isInBuffer(*lastBufferID))
	return *lastBufferID;
	}

	case GeneratedSourceInfo::ReplacedFunctionBody:
	// There's not really any "original" location for locations within
	// replaced function bodies. The body is actually different code to the
	// original file.
	case GeneratedSourceInfo::PrettyPrinted:
	case GeneratedSourceInfo::AttributeFromClang:
	// No original location, return the original buffer/location
	return {startBufferID, startRange};

[SourceKit] Support location info for macro-expanded Clang imports #82006

[SourceKit] Support location info for macro-expanded Clang imports #82006

Uh oh!

Conversation

j-hui commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

j-hui commented Jun 5, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hamishknight commented Jun 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnbarham commented Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnbarham Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

j-hui commented Jun 6, 2025

Uh oh!

j-hui commented Jun 6, 2025

Uh oh!

j-hui commented Jun 6, 2025

Uh oh!

j-hui commented Jun 10, 2025

Uh oh!

j-hui commented Jun 10, 2025

Uh oh!

j-hui commented Jun 11, 2025

Uh oh!

hnrklssn left a comment

Choose a reason for hiding this comment

Uh oh!

j-hui commented Jun 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

j-hui Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnbarham Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

j-hui commented Jun 13, 2025

Uh oh!

j-hui commented Jun 13, 2025

Uh oh!

j-hui commented Jun 13, 2025

Uh oh!

j-hui commented Jun 14, 2025

Uh oh!

j-hui commented Jun 16, 2025

j-hui commented Jun 5, 2025 •

edited

Loading

bnbarham commented Jun 6, 2025 •

edited

Loading

bnbarham Jun 6, 2025 •

edited

Loading

j-hui Jun 13, 2025 •

edited

Loading

bnbarham Jun 13, 2025 •

edited

Loading