Skip to content

Commit 54aab98

Browse files
atrickatrick
committed
Fix MoveOnlyDiagnostics, ConsumOperator...Checkers diagnostics
Emitting a note with an invalid source location is actively harmful. It confuses users and tools, makes it impossible to write unit tests. In this case, the note simply says "use here", so it's completely free of information without the source location. (cherry picked from commit a99ea62)
1 parent 662039b commit 54aab98

File tree

3 files changed

+51
-19
lines changed

3 files changed

+51
-19
lines changed

‎lib/SILOptimizer/Mandatory/ConsumeOperatorCopyableAddressesChecker.cpp

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1660,12 +1660,16 @@ bool DataflowState::process(
16601660

16611661
{
16621662
auto diag = diag::sil_movechecking_consuming_use_here;
1663-
diagnose(astContext, mvi->getLoc().getSourceLoc(), diag);
1663+
if (auto sourceLoc = mvi->getLoc().getSourceLoc()) {
1664+
diagnose(astContext, sourceLoc, diag);
1665+
}
16641666
}
16651667

16661668
{
16671669
auto diag = diag::sil_movechecking_nonconsuming_use_here;
1668-
diagnose(astContext, iter->second->getLoc().getSourceLoc(), diag);
1670+
if (auto sourceLoc = iter->second->getLoc().getSourceLoc()) {
1671+
diagnose(astContext, sourceLoc, diag);
1672+
}
16691673
}
16701674

16711675
emittedSingleDiagnostic = true;
@@ -1690,13 +1694,17 @@ bool DataflowState::process(
16901694

16911695
{
16921696
auto diag = diag::sil_movechecking_consuming_use_here;
1693-
diagnose(astContext, mvi->getLoc().getSourceLoc(), diag);
1697+
if (auto sourceLoc = mvi->getLoc().getSourceLoc()) {
1698+
diagnose(astContext, sourceLoc, diag);
1699+
}
16941700
}
16951701

16961702
{
16971703
auto diag = diag::sil_movechecking_nonconsuming_use_here;
16981704
for (auto *user : iter->second->pairedUseInsts) {
1699-
diagnose(astContext, user->getLoc().getSourceLoc(), diag);
1705+
if (auto sourceLoc = user->getLoc().getSourceLoc()) {
1706+
diagnose(astContext, sourceLoc, diag);
1707+
}
17001708
}
17011709
}
17021710

@@ -2115,12 +2123,16 @@ bool ConsumeOperatorCopyableAddressesChecker::performSingleBasicBlockAnalysis(
21152123
}
21162124

21172125
auto diag = diag::sil_movechecking_consuming_use_here;
2118-
diagnose(astCtx, mvi->getLoc().getSourceLoc(), diag);
2126+
if (auto sourceLoc = mvi->getLoc().getSourceLoc()) {
2127+
diagnose(astCtx, sourceLoc, diag);
2128+
}
21192129

21202130
{
21212131
auto diag = diag::sil_movechecking_nonconsuming_use_here;
21222132
for (auto *user : interestingClosureUsers) {
2123-
diagnose(astCtx, user->getLoc().getSourceLoc(), diag);
2133+
if (auto sourceLoc = user->getLoc().getSourceLoc()) {
2134+
diagnose(astCtx, sourceLoc, diag);
2135+
}
21242136
}
21252137
}
21262138

@@ -2157,12 +2169,16 @@ bool ConsumeOperatorCopyableAddressesChecker::performSingleBasicBlockAnalysis(
21572169

21582170
{
21592171
auto diag = diag::sil_movechecking_consuming_use_here;
2160-
diagnose(astCtx, mvi->getLoc().getSourceLoc(), diag);
2172+
if (auto sourceLoc = mvi->getLoc().getSourceLoc()) {
2173+
diagnose(astCtx, sourceLoc, diag);
2174+
}
21612175
}
21622176

21632177
{
21642178
auto diag = diag::sil_movechecking_nonconsuming_use_here;
2165-
diagnose(astCtx, interestingUser->getLoc().getSourceLoc(), diag);
2179+
if (auto sourceLoc = interestingUser->getLoc().getSourceLoc()) {
2180+
diagnose(astCtx, sourceLoc, diag);
2181+
}
21662182
}
21672183

21682184
// We purposely continue to see if at least in simple cases, we can flag

‎lib/SILOptimizer/Mandatory/ConsumeOperatorCopyableValuesChecker.cpp

Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -252,8 +252,10 @@ void ConsumeOperatorCopyableValuesChecker::emitDiagnosticForMove(
252252
diagnose(astContext, getSourceLocFromValue(borrowedValue),
253253
diag::sil_movechecking_value_used_after_consume,
254254
borrowedValueName);
255-
diagnose(astContext, mvi->getLoc().getSourceLoc(),
256-
diag::sil_movechecking_consuming_use_here);
255+
if (auto sourceLoc = mvi->getLoc().getSourceLoc()) {
256+
diagnose(astContext, sourceLoc,
257+
diag::sil_movechecking_consuming_use_here);
258+
}
257259

258260
// Then we do a bit of work to figure out where /all/ of the later uses than
259261
// mvi are so we can emit notes to the user telling them this is a problem
@@ -283,8 +285,10 @@ void ConsumeOperatorCopyableValuesChecker::emitDiagnosticForMove(
283285
case PrunedLiveness::NonLifetimeEndingUse:
284286
case PrunedLiveness::LifetimeEndingUse:
285287
LLVM_DEBUG(llvm::dbgs() << "Emitting note for in block use: " << inst);
286-
diagnose(astContext, inst.getLoc().getSourceLoc(),
287-
diag::sil_movechecking_nonconsuming_use_here);
288+
if (auto sourceLoc = inst.getLoc().getSourceLoc()) {
289+
diagnose(astContext, sourceLoc,
290+
diag::sil_movechecking_nonconsuming_use_here);
291+
}
288292
break;
289293
}
290294
}
@@ -342,8 +346,10 @@ void ConsumeOperatorCopyableValuesChecker::emitDiagnosticForMove(
342346
case PrunedLiveness::LifetimeEndingUse:
343347
LLVM_DEBUG(llvm::dbgs()
344348
<< "(3) Emitting diagnostic for user: " << inst);
345-
diagnose(astContext, inst.getLoc().getSourceLoc(),
346-
diag::sil_movechecking_nonconsuming_use_here);
349+
if (auto sourceLoc = inst.getLoc().getSourceLoc()) {
350+
diagnose(astContext, sourceLoc,
351+
diag::sil_movechecking_nonconsuming_use_here);
352+
}
347353
break;
348354
}
349355
}
@@ -368,8 +374,10 @@ void ConsumeOperatorCopyableValuesChecker::emitDiagnosticForMove(
368374
if (livenessInfo.nonLifetimeEndingUsesInLiveOut.contains(&inst)) {
369375
LLVM_DEBUG(llvm::dbgs()
370376
<< "(1) Emitting diagnostic for user: " << inst);
371-
diagnose(astContext, inst.getLoc().getSourceLoc(),
372-
diag::sil_movechecking_nonconsuming_use_here);
377+
if (auto sourceLoc = inst.getLoc().getSourceLoc()) {
378+
diagnose(astContext, sourceLoc,
379+
diag::sil_movechecking_nonconsuming_use_here);
380+
}
373381
continue;
374382
}
375383

@@ -390,8 +398,10 @@ void ConsumeOperatorCopyableValuesChecker::emitDiagnosticForMove(
390398

391399
LLVM_DEBUG(llvm::dbgs()
392400
<< "(2) Emitting diagnostic for user: " << inst);
393-
diagnose(astContext, inst.getLoc().getSourceLoc(),
394-
diag::sil_movechecking_nonconsuming_use_here);
401+
if (auto sourceLoc = inst.getLoc().getSourceLoc()) {
402+
diagnose(astContext, sourceLoc,
403+
diag::sil_movechecking_nonconsuming_use_here);
404+
}
395405
}
396406
}
397407
}

‎lib/SILOptimizer/Mandatory/MoveOnlyDiagnostics.cpp

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,13 @@ static void diagnose(ASTContext &context, SILInstruction *inst, Diag<T...> diag,
6060
if (SilentlyEmitDiagnostics)
6161
return;
6262

63-
context.Diags.diagnose(loc.getSourceLoc(), diag, std::forward<U>(args)...);
63+
auto sourceLoc = loc.getSourceLoc();
64+
auto diagKind = context.Diags.declaredDiagnosticKindFor(diag.ID);
65+
// Do not emit notes on invalid source locations.
66+
if (!sourceLoc && diagKind == DiagnosticKind::Note) {
67+
return;
68+
}
69+
context.Diags.diagnose(sourceLoc, diag, std::forward<U>(args)...);
6470
}
6571

6672
template <typename... T, typename... U>

0 commit comments

Comments
 (0)