Skip to content

Commit d590823

Browse files
beccadaxbeccadax
committed
Warn about conflicts between #file strings
1 parent 54f5967 commit d590823

File tree

7 files changed

+153
-6
lines changed

7 files changed

+153
-6
lines changed

‎include/swift/AST/DiagnosticsCommon.def

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,14 @@ ERROR(sdk_node_unrecognized_decl_kind,none,
139139
ERROR(sdk_node_unrecognized_accessor_kind,none,
140140
"unrecognized accessor kind '%0' in SDK node", (StringRef))
141141

142+
// Emitted from ModuleDecl::computeMagicFileStringMap()
143+
WARNING(pound_source_location_creates_pound_file_conflicts,none,
144+
"'#sourceLocation' directive produces '#file' string of '%0', which "
145+
"conflicts with '#file' strings produced by other paths in the module",
146+
(StringRef))
147+
NOTE(fixit_correct_source_location_file,none,
148+
"change file in '#sourceLocation' to '%0'", (StringRef))
149+
142150
//------------------------------------------------------------------------------
143151
// MARK: Circular reference diagnostics
144152
//------------------------------------------------------------------------------

‎include/swift/AST/Module.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -305,7 +305,7 @@ class ModuleDecl : public DeclContext, public TypeDecl {
305305
/// Note that this returns an empty StringMap if concise \c #file strings are
306306
/// disabled. Users should fall back to using the file path in this case.
307307
llvm::StringMap<std::pair<std::string, /*isWinner=*/bool>>
308-
computeMagicFileStringMap() const;
308+
computeMagicFileStringMap(bool shouldDiagnose) const;
309309

310310
/// Add a file declaring a cross-import overlay.
311311
void addCrossImportOverlayFile(StringRef file);

‎lib/AST/Module.cpp

Lines changed: 37 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2003,7 +2003,8 @@ computeMagicFileString(const ModuleDecl *module, StringRef name,
20032003

20042004
static StringRef
20052005
resolveMagicNameConflicts(const ModuleDecl *module, StringRef fileString,
2006-
const llvm::StringMap<SourceFilePathInfo> &paths) {
2006+
const llvm::StringMap<SourceFilePathInfo> &paths,
2007+
bool shouldDiagnose) {
20072008
assert(paths.size() > 1);
20082009
assert(module->getASTContext().LangOpts.EnableConcisePoundFile);
20092010

@@ -2026,11 +2027,43 @@ resolveMagicNameConflicts(const ModuleDecl *module, StringRef fileString,
20262027
}
20272028
}
20282029

2030+
// If we're not diagnosing, that's all we need to do.
2031+
if (!shouldDiagnose)
2032+
return winner;
2033+
2034+
SmallString<64> winnerLiteral;
2035+
llvm::raw_svector_ostream winnerLiteralStream{winnerLiteral};
2036+
swift::printAsQuotedString(winnerLiteralStream, winner);
2037+
2038+
auto &diags = module->getASTContext().Diags;
2039+
2040+
// Diagnose the conflict at each #sourceLocation that specifies it.
2041+
for (const auto &pathPair : paths) {
2042+
bool isWinner = (pathPair.first() == winner);
2043+
2044+
// Don't diagnose #sourceLocations that match the physical file.
2045+
if (pathPair.second.physicalFileLoc.isValid()) {
2046+
assert(isWinner && "physical files should always win; duplicate name?");
2047+
continue;
2048+
}
2049+
2050+
for (auto loc : pathPair.second.virtualFileLocs) {
2051+
diags.diagnose(loc,
2052+
diag::pound_source_location_creates_pound_file_conflicts,
2053+
fileString);
2054+
2055+
// Offer a fix-it unless it would be tautological.
2056+
if (!isWinner)
2057+
diags.diagnose(loc, diag::fixit_correct_source_location_file, winner)
2058+
.fixItReplace(loc, winnerLiteral);
2059+
}
2060+
}
2061+
20292062
return winner;
20302063
}
20312064

20322065
llvm::StringMap<std::pair<std::string, bool>>
2033-
ModuleDecl::computeMagicFileStringMap() const {
2066+
ModuleDecl::computeMagicFileStringMap(bool shouldDiagnose) const {
20342067
llvm::StringMap<std::pair<std::string, bool>> result;
20352068
SmallString<64> scratch;
20362069

@@ -2048,7 +2081,8 @@ ModuleDecl::computeMagicFileStringMap() const {
20482081
// will simply warn about conflicts.
20492082
StringRef winner = infoForPaths.begin()->first();
20502083
if (infoForPaths.size() > 1)
2051-
winner = resolveMagicNameConflicts(this, scratch, infoForPaths);
2084+
winner = resolveMagicNameConflicts(this, scratch, infoForPaths,
2085+
shouldDiagnose);
20522086

20532087
for (auto &pathPair : infoForPaths) {
20542088
result[pathPair.first()] =

‎lib/SIL/SILPrinter.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2918,7 +2918,7 @@ void SILModule::print(SILPrintContext &PrintCtx, ModuleDecl *M,
29182918

29192919
if (M)
29202920
printMagicFileStringMap(
2921-
PrintCtx, M->computeMagicFileStringMap());
2921+
PrintCtx, M->computeMagicFileStringMap(/*shouldDiagnose=*/false));
29222922

29232923
OS << "\n\n";
29242924
}

‎lib/SILGen/SILGen.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,8 @@ using namespace Lowering;
5151

5252
SILGenModule::SILGenModule(SILModule &M, ModuleDecl *SM)
5353
: M(M), Types(M.Types), SwiftModule(SM), TopLevelSGF(nullptr),
54-
MagicFileStringsByFilePath(SM->computeMagicFileStringMap()) {
54+
MagicFileStringsByFilePath(
55+
SM->computeMagicFileStringMap(/*shouldDiagnose=*/true)) {
5556
const SILOptions &Opts = M.getOptions();
5657
if (!Opts.UseProfile.empty()) {
5758
auto ReaderOrErr = llvm::IndexedInstrProfReader::create(Opts.UseProfile);

‎test/SILGen/Inputs/magic_identifier_file_conflicting_other.swift

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
// This file matches test/SILGen/magic_identifier_file_conflicting.swift.
2+
// It should be compiled with -verify.
3+
4+
// We should diagnose cross-file conflicts.
5+
// expected-warning@+2 {{'#sourceLocation' directive produces '#file' string of 'Foo/other_file_c.swift', which conflicts with '#file' strings produced by other paths in the module}}
6+
// expected-note@+1 {{change file in '#sourceLocation' to 'first/other_file_c.swift'}} {{23-50="first/other_file_c.swift"}}
7+
#sourceLocation(file: "second/other_file_c.swift", line: 1)
8+
#sourceLocation()

‎test/SILGen/magic_identifier_file_conflicting.swift.gyb

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
// RUN: %empty-directory(%t)
2+
// RUN: %gyb -D TEMPDIR=%t %s > %t/magic_identifier_file_conflicting.swift
3+
4+
// We want to check both the diagnostics and the SIL output.
5+
// RUN: %target-swift-emit-silgen -verify -emit-sorted-sil -enable-experimental-concise-pound-file -module-name Foo %t/magic_identifier_file_conflicting.swift %S/Inputs/magic_identifier_file_conflicting_other.swift | %FileCheck %s
6+
7+
%{
8+
TEMPDIR_ESC = TEMPDIR.replace('\\', '\\\\')
9+
def fixit_loc(start_col, orig_suffix):
10+
"""
11+
Computes a "start-end" string for a fix-it replacing a string literal which
12+
starts at start_col and joins orig_suffix to TEMPDIR with a slash.
13+
"""
14+
original = '"{0}/{1}"'.format(TEMPDIR, orig_suffix)
15+
end_col = start_col + len(original)
16+
return '{0}-{1}'.format(start_col, end_col)
17+
}%
18+
19+
//
20+
// Same name as a physical file
21+
//
22+
23+
// There should be no warning for the exact same path.
24+
// no-warning
25+
#sourceLocation(file: "${TEMPDIR_ESC}/magic_identifier_file_conflicting.swift", line: 10)
26+
#sourceLocation()
27+
28+
// expected-warning@+2 {{'#sourceLocation' directive produces '#file' string of 'Foo/magic_identifier_file_conflicting.swift', which conflicts with '#file' strings produced by other paths in the module}}
29+
// expected-note@+1 {{change file in '#sourceLocation' to '${TEMPDIR_ESC}/magic_identifier_file_conflicting.swift'}} {{${fixit_loc(23, "other_path_b/magic_identifier_file_conflicting.swift")}="${TEMPDIR_ESC}/magic_identifier_file_conflicting.swift"}}
30+
#sourceLocation(file: "${TEMPDIR_ESC}/other_path_b/magic_identifier_file_conflicting.swift", line: 20)
31+
#sourceLocation()
32+
33+
// expected-warning@+2 {{'#sourceLocation' directive produces '#file' string of 'Foo/magic_identifier_file_conflicting.swift', which conflicts with '#file' strings produced by other paths in the module}}
34+
// expected-note@+1 {{change file in '#sourceLocation' to '${TEMPDIR_ESC}/magic_identifier_file_conflicting.swift'}} {{23-64="${TEMPDIR_ESC}/magic_identifier_file_conflicting.swift"}}
35+
#sourceLocation(file: "magic_identifier_file_conflicting.swift", line: 30)
36+
#sourceLocation()
37+
38+
//
39+
// No physical file with the same name
40+
//
41+
42+
// There shoud be no warning for a purely virtual file.
43+
// no-warning
44+
#sourceLocation(file: "other_file_a.swift", line: 40)
45+
#sourceLocation()
46+
47+
// Even if you use it twice.
48+
// no-warning
49+
#sourceLocation(file: "other_file_a.swift", line: 50)
50+
#sourceLocation()
51+
52+
// But there should be warnings for different-path, same-name virtual files.
53+
// The lexicographically first path should be treated as canonical--we diagnose
54+
// but don't offer a fix-it.
55+
// expected-warning@+1 {{'#sourceLocation' directive produces '#file' string of 'Foo/other_file_b.swift', which conflicts with '#file' strings produced by other paths in the module}}
56+
#sourceLocation(file: "first/other_file_b.swift", line: 60)
57+
#sourceLocation()
58+
59+
// Subsequent paths should fix-it to the first one.
60+
// expected-warning@+2 {{'#sourceLocation' directive produces '#file' string of 'Foo/other_file_b.swift', which conflicts with '#file' strings produced by other paths in the module}}
61+
// expected-note@+1 {{change file in '#sourceLocation' to 'first/other_file_b.swift'}} {{23-50="first/other_file_b.swift"}}
62+
#sourceLocation(file: "second/other_file_b.swift", line: 70)
63+
#sourceLocation()
64+
65+
// Even if there's more than one.
66+
// expected-warning@+2 {{'#sourceLocation' directive produces '#file' string of 'Foo/other_file_b.swift', which conflicts with '#file' strings produced by other paths in the module}}
67+
// expected-note@+1 {{change file in '#sourceLocation' to 'first/other_file_b.swift'}} {{23-49="first/other_file_b.swift"}}
68+
#sourceLocation(file: "third/other_file_b.swift", line: 80)
69+
#sourceLocation()
70+
71+
// Even if one is duplicated.
72+
// expected-warning@+2 {{'#sourceLocation' directive produces '#file' string of 'Foo/other_file_b.swift', which conflicts with '#file' strings produced by other paths in the module}}
73+
// expected-note@+1 {{change file in '#sourceLocation' to 'first/other_file_b.swift'}} {{23-49="first/other_file_b.swift"}}
74+
#sourceLocation(file: "third/other_file_b.swift", line: 90)
75+
#sourceLocation()
76+
77+
// We should diagnose cross-file conflicts.
78+
// expected-warning@+1 {{'#sourceLocation' directive produces '#file' string of 'Foo/other_file_c.swift', which conflicts with '#file' strings produced by other paths in the module}}
79+
#sourceLocation(file: "first/other_file_c.swift", line: 100)
80+
#sourceLocation()
81+
82+
//
83+
// Check '#file' => '#filePath' mapping table
84+
//
85+
86+
// CHECK-LABEL: // Mappings from '#file' to '#filePath':
87+
// CHECK-NEXT: // 'Foo/magic_identifier_file_conflicting.swift' => 'BUILD_DIR{{[/\\]}}test-{{[^/]+}}{{[/\\]}}SILGen{{[/\\]}}Output{{[/\\]}}magic_identifier_file_conflicting.swift.gyb.tmp{{[/\\]}}magic_identifier_file_conflicting.swift'
88+
// CHECK-NEXT: // 'Foo/magic_identifier_file_conflicting.swift' => 'BUILD_DIR{{[/\\]}}test-{{[^/]+}}{{[/\\]}}SILGen{{[/\\]}}Output{{[/\\]}}magic_identifier_file_conflicting.swift.gyb.tmp{{[/\\]}}other_path_b{{[/\\]}}magic_identifier_file_conflicting.swift' (alternate)
89+
// CHECK-NEXT: // 'Foo/magic_identifier_file_conflicting.swift' => 'magic_identifier_file_conflicting.swift' (alternate)
90+
// CHECK-NEXT: // 'Foo/magic_identifier_file_conflicting_other.swift' => 'SOURCE_DIR{{[/\\]}}test{{[/\\]}}SILGen{{[/\\]}}Inputs{{[/\\]}}magic_identifier_file_conflicting_other.swift'
91+
// CHECK-NEXT: // 'Foo/other_file_a.swift' => 'other_file_a.swift'
92+
// CHECK-NEXT: // 'Foo/other_file_b.swift' => 'first/other_file_b.swift'
93+
// CHECK-NEXT: // 'Foo/other_file_b.swift' => 'second/other_file_b.swift' (alternate)
94+
// CHECK-NEXT: // 'Foo/other_file_b.swift' => 'third/other_file_b.swift' (alternate)
95+
// CHECK-NEXT: // 'Foo/other_file_c.swift' => 'first/other_file_c.swift'
96+
// CHECK-NEXT: // 'Foo/other_file_c.swift' => 'second/other_file_c.swift' (alternate)

0 commit comments

Comments
 (0)