Skip to content

Commit 5f26958

Browse files
jrose-applejrose-apple
authored
Merge pull request #26830 from owenv/diag_child_notes_refactor
[Diagnostics][NFC] Optionally add notes as explicit children of an error/warning
2 parents ef64b13 + 6e1e768 commit 5f26958

File tree

8 files changed

+196
-27
lines changed

8 files changed

+196
-27
lines changed

‎include/swift/AST/DiagnosticConsumer.h

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,10 +38,17 @@ enum class DiagnosticKind : uint8_t {
3838
Note
3939
};
4040

41-
/// Extra information carried along with a diagnostic, which may or
42-
/// may not be of interest to a given diagnostic consumer.
41+
/// Information about a diagnostic passed to DiagnosticConsumers.
4342
struct DiagnosticInfo {
4443
DiagID ID = DiagID(0);
44+
SourceLoc Loc;
45+
DiagnosticKind Kind;
46+
StringRef FormatString;
47+
ArrayRef<DiagnosticArgument> FormatArgs;
48+
SourceLoc BufferIndirectlyCausingDiagnostic;
49+
50+
/// DiagnosticInfo of notes which are children of this diagnostic, if any
51+
ArrayRef<DiagnosticInfo *> ChildDiagnosticInfo;
4552

4653
/// Represents a fix-it, a replacement of one range of text with another.
4754
class FixIt {
@@ -60,6 +67,24 @@ struct DiagnosticInfo {
6067

6168
/// Extra source ranges that are attached to the diagnostic.
6269
ArrayRef<FixIt> FixIts;
70+
71+
/// This is a note which has a parent error or warning
72+
bool IsChildNote = false;
73+
74+
DiagnosticInfo() {}
75+
76+
DiagnosticInfo(DiagID ID, SourceLoc Loc, DiagnosticKind Kind,
77+
StringRef FormatString,
78+
ArrayRef<DiagnosticArgument> FormatArgs,
79+
SourceLoc BufferIndirectlyCausingDiagnostic,
80+
ArrayRef<DiagnosticInfo *> ChildDiagnosticInfo,
81+
ArrayRef<CharSourceRange> Ranges, ArrayRef<FixIt> FixIts,
82+
bool IsChildNote)
83+
: ID(ID), Loc(Loc), Kind(Kind), FormatString(FormatString),
84+
FormatArgs(FormatArgs),
85+
BufferIndirectlyCausingDiagnostic(BufferIndirectlyCausingDiagnostic),
86+
ChildDiagnosticInfo(ChildDiagnosticInfo), Ranges(Ranges),
87+
FixIts(FixIts), IsChildNote(IsChildNote) {}
6388
};
6489

6590
/// Abstract interface for classes that present diagnostics to the user.

‎include/swift/AST/DiagnosticEngine.h

Lines changed: 78 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -337,7 +337,9 @@ namespace swift {
337337
SmallVector<DiagnosticArgument, 3> Args;
338338
SmallVector<CharSourceRange, 2> Ranges;
339339
SmallVector<FixIt, 2> FixIts;
340+
std::vector<Diagnostic> ChildNotes;
340341
SourceLoc Loc;
342+
bool IsChildNote = false;
341343
const Decl *Decl = nullptr;
342344

343345
friend DiagnosticEngine;
@@ -362,10 +364,13 @@ namespace swift {
362364
ArrayRef<DiagnosticArgument> getArgs() const { return Args; }
363365
ArrayRef<CharSourceRange> getRanges() const { return Ranges; }
364366
ArrayRef<FixIt> getFixIts() const { return FixIts; }
367+
ArrayRef<Diagnostic> getChildNotes() const { return ChildNotes; }
368+
bool isChildNote() const { return IsChildNote; }
365369
SourceLoc getLoc() const { return Loc; }
366370
const class Decl *getDecl() const { return Decl; }
367371

368372
void setLoc(SourceLoc loc) { Loc = loc; }
373+
void setIsChildNote(bool isChildNote) { IsChildNote = isChildNote; }
369374
void setDecl(const class Decl *decl) { Decl = decl; }
370375

371376
/// Returns true if this object represents a particular diagnostic.
@@ -386,6 +391,8 @@ namespace swift {
386391
void addFixIt(FixIt &&F) {
387392
FixIts.push_back(std::move(F));
388393
}
394+
395+
void addChildNote(Diagnostic &&D);
389396
};
390397

391398
/// Describes an in-flight diagnostic, which is currently active
@@ -665,7 +672,8 @@ namespace swift {
665672

666673
friend class InFlightDiagnostic;
667674
friend class DiagnosticTransaction;
668-
675+
friend class CompoundDiagnosticTransaction;
676+
669677
public:
670678
explicit DiagnosticEngine(SourceManager &SourceMgr)
671679
: SourceMgr(SourceMgr), ActiveDiagnostic(),
@@ -878,6 +886,15 @@ namespace swift {
878886
return diagnose(decl, Diagnostic(id, std::move(args)...));
879887
}
880888

889+
/// Emit a parent diagnostic and attached notes.
890+
///
891+
/// \param parentDiag An InFlightDiagnostic representing the parent diag.
892+
///
893+
/// \param builder A closure which builds and emits notes to be attached to
894+
/// the parent diag.
895+
void diagnoseWithNotes(InFlightDiagnostic parentDiag,
896+
llvm::function_ref<void(void)> builder);
897+
881898
/// \returns true if diagnostic is marked with PointsToFirstBadToken
882899
/// option.
883900
bool isDiagnosticPointsToFirstBadToken(DiagID id) const;
@@ -905,6 +922,10 @@ namespace swift {
905922
/// Retrieve the active diagnostic.
906923
Diagnostic &getActiveDiagnostic() { return *ActiveDiagnostic; }
907924

925+
/// Generate DiagnosticInfo for a Diagnostic to be passed to consumers.
926+
Optional<DiagnosticInfo>
927+
diagnosticInfoForDiagnostic(const Diagnostic &diagnostic);
928+
908929
/// Send \c diag to all diagnostic consumers.
909930
void emitDiagnostic(const Diagnostic &diag);
910931

@@ -945,6 +966,7 @@ namespace swift {
945966
/// in LIFO order. An open transaction is implicitly committed upon
946967
/// destruction.
947968
class DiagnosticTransaction {
969+
protected:
948970
DiagnosticEngine &Engine;
949971

950972
/// How many tentative diagnostics there were when the transaction
@@ -968,7 +990,6 @@ namespace swift {
968990
Depth(Engine.TransactionCount),
969991
IsOpen(true)
970992
{
971-
assert(!Engine.ActiveDiagnostic);
972993
Engine.TransactionCount++;
973994
}
974995

@@ -1011,6 +1032,61 @@ namespace swift {
10111032
"transactions must be closed LIFO");
10121033
}
10131034
};
1035+
1036+
/// Represents a diagnostic transaction which constructs a compound diagnostic
1037+
/// from any diagnostics emitted inside. A compound diagnostic consists of a
1038+
/// parent error, warning, or remark followed by a variable number of child
1039+
/// notes. The semantics are otherwise the same as a regular
1040+
/// DiagnosticTransaction.
1041+
class CompoundDiagnosticTransaction : public DiagnosticTransaction {
1042+
public:
1043+
explicit CompoundDiagnosticTransaction(DiagnosticEngine &engine)
1044+
: DiagnosticTransaction(engine) {}
1045+
1046+
~CompoundDiagnosticTransaction() {
1047+
if (IsOpen) {
1048+
commit();
1049+
}
1050+
1051+
if (Depth == 0) {
1052+
Engine.TransactionStrings.clear();
1053+
Engine.TransactionAllocator.Reset();
1054+
}
1055+
}
1056+
1057+
void commit() {
1058+
assert(PrevDiagnostics < Engine.TentativeDiagnostics.size() &&
1059+
"CompoundDiagnosticTransaction must contain at least one diag");
1060+
1061+
// The first diagnostic is assumed to be the parent. If this is not an
1062+
// error or warning, we'll assert later when trying to add children.
1063+
Diagnostic &parent = Engine.TentativeDiagnostics[PrevDiagnostics];
1064+
1065+
// Associate the children with the parent.
1066+
for (auto diag =
1067+
Engine.TentativeDiagnostics.begin() + PrevDiagnostics + 1;
1068+
diag != Engine.TentativeDiagnostics.end(); ++diag) {
1069+
diag->setIsChildNote(true);
1070+
parent.addChildNote(std::move(*diag));
1071+
}
1072+
1073+
// Erase the children, they'll be emitted alongside their parent.
1074+
Engine.TentativeDiagnostics.erase(Engine.TentativeDiagnostics.begin() +
1075+
PrevDiagnostics + 1,
1076+
Engine.TentativeDiagnostics.end());
1077+
1078+
DiagnosticTransaction::commit();
1079+
}
1080+
};
1081+
1082+
inline void
1083+
DiagnosticEngine::diagnoseWithNotes(InFlightDiagnostic parentDiag,
1084+
llvm::function_ref<void(void)> builder) {
1085+
CompoundDiagnosticTransaction transaction(*this);
1086+
parentDiag.flush();
1087+
builder();
1088+
}
1089+
10141090
} // end namespace swift
10151091

10161092
#endif

‎include/swift/Frontend/PrintingDiagnosticConsumer.h

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,13 @@ class PrintingDiagnosticConsumer : public DiagnosticConsumer {
5050
bool didErrorOccur() {
5151
return DidErrorOccur;
5252
}
53+
54+
private:
55+
void printDiagnostic(SourceManager &SM, SourceLoc Loc, DiagnosticKind Kind,
56+
StringRef FormatString,
57+
ArrayRef<DiagnosticArgument> FormatArgs,
58+
const DiagnosticInfo &Info,
59+
SourceLoc bufferIndirectlyCausingDiagnostic);
5360
};
5461

5562
}

‎lib/AST/DiagnosticEngine.cpp

Lines changed: 42 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -279,6 +279,14 @@ void InFlightDiagnostic::flush() {
279279
Engine->flushActiveDiagnostic();
280280
}
281281

282+
void Diagnostic::addChildNote(Diagnostic &&D) {
283+
assert(storedDiagnosticInfos[(unsigned)D.ID].kind == DiagnosticKind::Note &&
284+
"Only notes can have a parent.");
285+
assert(storedDiagnosticInfos[(unsigned)ID].kind != DiagnosticKind::Note &&
286+
"Notes can't have children.");
287+
ChildNotes.push_back(std::move(D));
288+
}
289+
282290
bool DiagnosticEngine::isDiagnosticPointsToFirstBadToken(DiagID ID) const {
283291
return storedDiagnosticInfos[(unsigned) ID].pointsToFirstBadToken;
284292
}
@@ -803,10 +811,11 @@ void DiagnosticEngine::emitTentativeDiagnostics() {
803811
TentativeDiagnostics.clear();
804812
}
805813

806-
void DiagnosticEngine::emitDiagnostic(const Diagnostic &diagnostic) {
814+
Optional<DiagnosticInfo>
815+
DiagnosticEngine::diagnosticInfoForDiagnostic(const Diagnostic &diagnostic) {
807816
auto behavior = state.determineBehavior(diagnostic.getID());
808817
if (behavior == DiagnosticState::Behavior::Ignore)
809-
return;
818+
return None;
810819

811820
// Figure out the source location.
812821
SourceLoc loc = diagnostic.getLoc();
@@ -846,7 +855,7 @@ void DiagnosticEngine::emitDiagnostic(const Diagnostic &diagnostic) {
846855
// FIXME: Horrible, horrible hackaround. We're not getting a
847856
// DeclContext everywhere we should.
848857
if (!dc) {
849-
return;
858+
return None;
850859
}
851860

852861
while (!dc->isModuleContext()) {
@@ -923,17 +932,37 @@ void DiagnosticEngine::emitDiagnostic(const Diagnostic &diagnostic) {
923932
}
924933
}
925934

926-
// Pass the diagnostic off to the consumer.
927-
DiagnosticInfo Info;
928-
Info.ID = diagnostic.getID();
929-
Info.Ranges = diagnostic.getRanges();
930-
Info.FixIts = diagnostic.getFixIts();
931-
for (auto &Consumer : Consumers) {
932-
Consumer->handleDiagnostic(
933-
SourceMgr, loc, toDiagnosticKind(behavior),
934-
diagnosticStringFor(Info.ID, getPrintDiagnosticNames()),
935-
diagnostic.getArgs(), Info, getDefaultDiagnosticLoc());
935+
return DiagnosticInfo(
936+
diagnostic.getID(), loc, toDiagnosticKind(behavior),
937+
diagnosticStringFor(diagnostic.getID(), getPrintDiagnosticNames()),
938+
diagnostic.getArgs(), getDefaultDiagnosticLoc(), /*child note info*/ {},
939+
diagnostic.getRanges(), diagnostic.getFixIts(), diagnostic.isChildNote());
940+
}
941+
942+
void DiagnosticEngine::emitDiagnostic(const Diagnostic &diagnostic) {
943+
if (auto info = diagnosticInfoForDiagnostic(diagnostic)) {
944+
SmallVector<DiagnosticInfo, 1> childInfo;
945+
TinyPtrVector<DiagnosticInfo *> childInfoPtrs;
946+
auto childNotes = diagnostic.getChildNotes();
947+
for (unsigned idx = 0; idx < childNotes.size(); ++idx) {
948+
if (auto child = diagnosticInfoForDiagnostic(childNotes[idx])) {
949+
childInfo.push_back(*child);
950+
childInfoPtrs.push_back(&childInfo[idx]);
951+
}
952+
}
953+
info->ChildDiagnosticInfo = childInfoPtrs;
954+
for (auto &consumer : Consumers) {
955+
consumer->handleDiagnostic(SourceMgr, info->Loc, info->Kind,
956+
info->FormatString, info->FormatArgs, *info,
957+
info->BufferIndirectlyCausingDiagnostic);
958+
}
936959
}
960+
961+
// For compatibility with DiagnosticConsumers which don't know about child
962+
// notes. These can be ignored by consumers which do take advantage of the
963+
// grouping.
964+
for (auto &childNote : diagnostic.getChildNotes())
965+
emitDiagnostic(childNote);
937966
}
938967

939968
const char *DiagnosticEngine::diagnosticStringFor(const DiagID id,

‎lib/Frontend/PrintingDiagnosticConsumer.cpp

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,25 @@ void PrintingDiagnosticConsumer::handleDiagnostic(
6868
StringRef FormatString, ArrayRef<DiagnosticArgument> FormatArgs,
6969
const DiagnosticInfo &Info,
7070
const SourceLoc bufferIndirectlyCausingDiagnostic) {
71+
if (Info.IsChildNote)
72+
return;
73+
74+
printDiagnostic(SM, Loc, Kind, FormatString, FormatArgs, Info,
75+
bufferIndirectlyCausingDiagnostic);
76+
77+
for (auto ChildInfo : Info.ChildDiagnosticInfo) {
78+
printDiagnostic(SM, ChildInfo->Loc, ChildInfo->Kind,
79+
ChildInfo->FormatString, ChildInfo->FormatArgs, *ChildInfo,
80+
ChildInfo->BufferIndirectlyCausingDiagnostic);
81+
}
82+
}
83+
84+
void PrintingDiagnosticConsumer::printDiagnostic(
85+
SourceManager &SM, SourceLoc Loc, DiagnosticKind Kind,
86+
StringRef FormatString, ArrayRef<DiagnosticArgument> FormatArgs,
87+
const DiagnosticInfo &Info,
88+
const SourceLoc bufferIndirectlyCausingDiagnostic) {
89+
7190
// Determine what kind of diagnostic we're emitting.
7291
llvm::SourceMgr::DiagKind SMKind;
7392
switch (Kind) {

‎lib/Sema/CSApply.cpp

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2510,15 +2510,19 @@ namespace {
25102510
auto baseTyUnwrappedName = baseTyUnwrapped->getString();
25112511
auto loc = DSCE->getLoc();
25122512
auto startLoc = DSCE->getStartLoc();
2513-
2514-
tc.diagnose(loc, swift::diag::optional_ambiguous_case_ref,
2515-
baseTyName, baseTyUnwrappedName, memberName.str());
2516-
2517-
tc.diagnose(loc, swift::diag::optional_fixit_ambiguous_case_ref)
2518-
.fixItInsert(startLoc, "Optional");
2519-
tc.diagnose(loc, swift::diag::type_fixit_optional_ambiguous_case_ref,
2513+
tc.diagnoseWithNotes(
2514+
tc.diagnose(loc, swift::diag::optional_ambiguous_case_ref,
2515+
baseTyName, baseTyUnwrappedName, memberName.str()),
2516+
[&]() {
2517+
tc.diagnose(loc,
2518+
swift::diag::optional_fixit_ambiguous_case_ref)
2519+
.fixItInsert(startLoc, "Optional");
2520+
tc.diagnose(
2521+
loc,
2522+
swift::diag::type_fixit_optional_ambiguous_case_ref,
25202523
baseTyUnwrappedName, memberName.str())
2521-
.fixItInsert(startLoc, baseTyUnwrappedName);
2524+
.fixItInsert(startLoc, baseTyUnwrappedName);
2525+
});
25222526
}
25232527
}
25242528
}

‎lib/Sema/TypeCheckDecl.cpp

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -869,8 +869,10 @@ static void checkRedeclaration(TypeChecker &tc, ValueDecl *current) {
869869
current->getFullName(),
870870
otherInit->isMemberwiseInitializer());
871871
} else {
872-
tc.diagnose(current, diag::invalid_redecl, current->getFullName());
873-
tc.diagnose(other, diag::invalid_redecl_prev, other->getFullName());
872+
tc.diagnoseWithNotes(tc.diagnose(current, diag::invalid_redecl,
873+
current->getFullName()), [&]() {
874+
tc.diagnose(other, diag::invalid_redecl_prev, other->getFullName());
875+
});
874876
}
875877
markInvalid();
876878
}

‎lib/Sema/TypeChecker.h

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -732,6 +732,13 @@ class TypeChecker final : public LazyResolver {
732732
return Diags.diagnose(std::forward<ArgTypes>(Args)...);
733733
}
734734

735+
void diagnoseWithNotes(InFlightDiagnostic parentDiag,
736+
llvm::function_ref<void(void)> builder) {
737+
CompoundDiagnosticTransaction transaction(Diags);
738+
parentDiag.flush();
739+
builder();
740+
}
741+
735742
static Type getArraySliceType(SourceLoc loc, Type elementType);
736743
static Type getDictionaryType(SourceLoc loc, Type keyType, Type valueType);
737744
static Type getOptionalType(SourceLoc loc, Type elementType);

0 commit comments

Comments
 (0)