From b55c4f1fc9e2dcf73b61d2db19e97db694d61af1 Mon Sep 17 00:00:00 2001 From: Kavon Farvardin Date: Sun, 12 Nov 2023 21:54:55 -0800 Subject: [PATCH 1/2] [nfc] allow ErrorTypeRepr to store a ZeroArgDiagnostic There are sometimes parsing stuations where we don't want to emit a parsing error, because of feature guarding. For example, if a Feature involves new syntax for a type, we must be able to parse both the true and false sides of an ifdef guarding that new syntax based on a Feature flag. --- include/swift/AST/DiagnosticEngine.h | 3 +++ include/swift/AST/TypeRepr.h | 33 ++++++++++++++++++++++------ lib/AST/DiagnosticEngine.cpp | 3 +++ lib/AST/TypeRepr.cpp | 9 ++++++++ lib/Parse/ParseDecl.cpp | 2 +- lib/Parse/ParseGeneric.cpp | 8 +++---- lib/Parse/ParsePattern.cpp | 4 ++-- lib/Parse/ParseType.cpp | 12 +++++----- lib/Sema/PreCheckExpr.cpp | 6 ++--- lib/Sema/TypeCheckType.cpp | 1 + 10 files changed, 58 insertions(+), 23 deletions(-) diff --git a/include/swift/AST/DiagnosticEngine.h b/include/swift/AST/DiagnosticEngine.h index 908c9979f1d40..c956bd415d12a 100644 --- a/include/swift/AST/DiagnosticEngine.h +++ b/include/swift/AST/DiagnosticEngine.h @@ -509,6 +509,9 @@ namespace swift { void addChildNote(Diagnostic &&D); void insertChildNote(unsigned beforeIndex, Diagnostic &&D); }; + + /// A diagnostic that has no input arguments, so it is trivially-destructable. + using ZeroArgDiagnostic = Diag<>; /// Describes an in-flight diagnostic, which is currently active /// within the diagnostic engine and can be augmented within additional diff --git a/include/swift/AST/TypeRepr.h b/include/swift/AST/TypeRepr.h index c882e2e4395d9..36c5b6da49ec9 100644 --- a/include/swift/AST/TypeRepr.h +++ b/include/swift/AST/TypeRepr.h @@ -19,6 +19,7 @@ #include "swift/AST/Attr.h" #include "swift/AST/DeclContext.h" +#include "swift/AST/DiagnosticEngine.h" #include "swift/AST/GenericSignature.h" #include "swift/AST/Identifier.h" #include "swift/AST/Type.h" @@ -195,17 +196,35 @@ class alignas(1 << TypeReprAlignInBits) TypeRepr /// A TypeRepr for a type with a syntax error. Can be used both as a /// top-level TypeRepr and as a part of other TypeRepr. /// -/// The client should make sure to emit a diagnostic at the construction time -/// (in the parser). All uses of this type should be ignored and not -/// re-diagnosed. +/// The client can either emit a detailed diagnostic at the construction time +/// (in the parser) or store a zero-arg diagnostic in this TypeRepr to be +/// emitted after parsing, during type resolution. +/// +/// All uses of this type should be ignored and not re-diagnosed. class ErrorTypeRepr : public TypeRepr { SourceRange Range; + llvm::Optional DelayedDiag; + + ErrorTypeRepr(SourceRange Range, llvm::Optional Diag) + : TypeRepr(TypeReprKind::Error), Range(Range), DelayedDiag(Diag) {} public: - ErrorTypeRepr() : TypeRepr(TypeReprKind::Error) {} - ErrorTypeRepr(SourceLoc Loc) : TypeRepr(TypeReprKind::Error), Range(Loc) {} - ErrorTypeRepr(SourceRange Range) - : TypeRepr(TypeReprKind::Error), Range(Range) {} + static ErrorTypeRepr * + create(ASTContext &Context, SourceRange Range, + llvm::Optional DelayedDiag = llvm::None) { + assert((!DelayedDiag || Range) && "diagnostic needs a location"); + return new (Context) ErrorTypeRepr(Range, DelayedDiag); + } + + static ErrorTypeRepr * + create(ASTContext &Context, SourceLoc Loc = SourceLoc(), + llvm::Optional DelayedDiag = llvm::None) { + return create(Context, SourceRange(Loc), DelayedDiag); + } + + /// If there is a delayed diagnostic stored in this TypeRepr, consumes and + /// emits that diagnostic. + void dischargeDiagnostic(ASTContext &Context); static bool classof(const TypeRepr *T) { return T->getKind() == TypeReprKind::Error; diff --git a/lib/AST/DiagnosticEngine.cpp b/lib/AST/DiagnosticEngine.cpp index d444f66075487..ccb4c1868e70e 100644 --- a/lib/AST/DiagnosticEngine.cpp +++ b/lib/AST/DiagnosticEngine.cpp @@ -42,6 +42,9 @@ using namespace swift; +static_assert(IsTriviallyDestructible::value, + "ZeroArgDiagnostic is meant to be trivially destructable"); + namespace { enum class DiagnosticOptions { /// No options. diff --git a/lib/AST/TypeRepr.cpp b/lib/AST/TypeRepr.cpp index 07a6b90bfc199..ac8f65af12de6 100644 --- a/lib/AST/TypeRepr.cpp +++ b/lib/AST/TypeRepr.cpp @@ -653,6 +653,15 @@ void SILBoxTypeRepr::printImpl(ASTPrinter &Printer, Printer.printKeyword("sil_box", Opts); } +void ErrorTypeRepr::dischargeDiagnostic(swift::ASTContext &Context) { + if (!DelayedDiag) + return; + + // Consume and emit the diagnostic. + Context.Diags.diagnose(Range.Start, *DelayedDiag).highlight(Range); + DelayedDiag = llvm::None; +} + // See swift/Basic/Statistic.h for declaration: this enables tracing // TypeReprs, is defined here to avoid too much layering violation / circular // linkage dependency. diff --git a/lib/Parse/ParseDecl.cpp b/lib/Parse/ParseDecl.cpp index 3ca9fd89818ee..19a9d47227b93 100644 --- a/lib/Parse/ParseDecl.cpp +++ b/lib/Parse/ParseDecl.cpp @@ -9665,7 +9665,7 @@ Parser::parseDeclSubscript(SourceLoc StaticLoc, if (ElementTy.isNull()) { // Always set an element type. - ElementTy = makeParserResult(ElementTy, new (Context) ErrorTypeRepr()); + ElementTy = makeParserResult(ElementTy, ErrorTypeRepr::create(Context)); } } diff --git a/lib/Parse/ParseGeneric.cpp b/lib/Parse/ParseGeneric.cpp index e1b669f9b9b22..2170b5c6c8892 100644 --- a/lib/Parse/ParseGeneric.cpp +++ b/lib/Parse/ParseGeneric.cpp @@ -339,7 +339,7 @@ ParserStatus Parser::parseGenericWhereClause( ParserResult Protocol = parseType(); Status |= Protocol; if (Protocol.isNull()) - Protocol = makeParserResult(new (Context) ErrorTypeRepr(PreviousLoc)); + Protocol = makeParserResult(ErrorTypeRepr::create(Context, PreviousLoc)); // Add the requirement. Requirements.push_back(RequirementRepr::getTypeConstraint( @@ -358,7 +358,7 @@ ParserStatus Parser::parseGenericWhereClause( ParserResult SecondType = parseType(); Status |= SecondType; if (SecondType.isNull()) - SecondType = makeParserResult(new (Context) ErrorTypeRepr(PreviousLoc)); + SecondType = makeParserResult(ErrorTypeRepr::create(Context, PreviousLoc)); // Add the requirement if (FirstType.hasCodeCompletion()) { @@ -373,7 +373,7 @@ ParserStatus Parser::parseGenericWhereClause( // completion token in the TypeRepr. Requirements.push_back(RequirementRepr::getTypeConstraint( FirstType.get(), EqualLoc, - new (Context) ErrorTypeRepr(SecondType.get()->getLoc()), + ErrorTypeRepr::create(Context, SecondType.get()->getLoc()), isRequirementExpansion)); } else { Requirements.push_back(RequirementRepr::getSameType( @@ -383,7 +383,7 @@ ParserStatus Parser::parseGenericWhereClause( } else if (FirstType.hasCodeCompletion()) { // Recover by adding dummy constraint. Requirements.push_back(RequirementRepr::getTypeConstraint( - FirstType.get(), PreviousLoc, new (Context) ErrorTypeRepr(PreviousLoc), + FirstType.get(), PreviousLoc, ErrorTypeRepr::create(Context, PreviousLoc), isRequirementExpansion)); } else { diagnose(Tok, diag::expected_requirement_delim); diff --git a/lib/Parse/ParsePattern.cpp b/lib/Parse/ParsePattern.cpp index 7e51b987cd5b7..dff019dccb5dd 100644 --- a/lib/Parse/ParsePattern.cpp +++ b/lib/Parse/ParsePattern.cpp @@ -1053,7 +1053,7 @@ ParserResult Parser::parseTypedPattern() { } } } else { - Ty = makeParserResult(new (Context) ErrorTypeRepr(PreviousLoc)); + Ty = makeParserResult(ErrorTypeRepr::create(Context, PreviousLoc)); } result = makeParserResult(result, @@ -1278,7 +1278,7 @@ parseOptionalPatternTypeAnnotation(ParserResult result) { TypeRepr *repr = Ty.getPtrOrNull(); if (!repr) - repr = new (Context) ErrorTypeRepr(PreviousLoc); + repr = ErrorTypeRepr::create(Context, PreviousLoc); return makeParserResult(status, new (Context) TypedPattern(P, repr)); } diff --git a/lib/Parse/ParseType.cpp b/lib/Parse/ParseType.cpp index ec1a22fbf03b8..4ba7efa36f1a7 100644 --- a/lib/Parse/ParseType.cpp +++ b/lib/Parse/ParseType.cpp @@ -232,7 +232,7 @@ ParserResult Parser::parseTypeSimple( CodeCompletionCallbacks->completeTypeSimpleBeginning(); } return makeParserCodeCompletionResult( - new (Context) ErrorTypeRepr(consumeToken(tok::code_complete))); + ErrorTypeRepr::create(Context, consumeToken(tok::code_complete))); case tok::l_square: { ty = parseTypeCollection(); break; @@ -255,7 +255,7 @@ ParserResult Parser::parseTypeSimple( diag.fixItInsert(getEndOfPreviousLoc(), " <#type#>"); } if (Tok.isKeyword() && !Tok.isAtStartOfLine()) { - ty = makeParserErrorResult(new (Context) ErrorTypeRepr(Tok.getLoc())); + ty = makeParserErrorResult(ErrorTypeRepr::create(Context, Tok.getLoc())); consumeToken(); return ty; } @@ -685,8 +685,8 @@ ParserResult Parser::parseDeclResultType(Diag<> MessageID) { auto diag = diagnose(Tok, diag::extra_rbracket); diag.fixItInsert(result.get()->getStartLoc(), getTokenText(tok::l_square)); consumeToken(); - return makeParserErrorResult(new (Context) - ErrorTypeRepr(getTypeErrorLoc())); + return makeParserErrorResult(ErrorTypeRepr::create(Context, + getTypeErrorLoc())); } if (Tok.is(tok::colon)) { @@ -702,8 +702,8 @@ ParserResult Parser::parseDeclResultType(Diag<> MessageID) { diag.fixItInsertAfter(secondType.get()->getEndLoc(), getTokenText(tok::r_square)); } } - return makeParserErrorResult(new (Context) - ErrorTypeRepr(getTypeErrorLoc())); + return makeParserErrorResult(ErrorTypeRepr::create(Context, + getTypeErrorLoc())); } } return result; diff --git a/lib/Sema/PreCheckExpr.cpp b/lib/Sema/PreCheckExpr.cpp index fe189ed3886f1..9f23b33598c19 100644 --- a/lib/Sema/PreCheckExpr.cpp +++ b/lib/Sema/PreCheckExpr.cpp @@ -2010,7 +2010,7 @@ TypeExpr *PreCheckExpression::simplifyTypeExpr(Expr *E) { Ctx.Diags.diagnose(AE->getArgsExpr()->getLoc(), diag::expected_type_before_arrow); auto ArgRange = AE->getArgsExpr()->getSourceRange(); - auto ErrRepr = new (Ctx) ErrorTypeRepr(ArgRange); + auto ErrRepr = ErrorTypeRepr::create(Ctx, ArgRange); ArgsTypeRepr = TupleTypeRepr::create(Ctx, {ErrRepr}, ArgRange); } @@ -2025,8 +2025,8 @@ TypeExpr *PreCheckExpression::simplifyTypeExpr(Expr *E) { if (!ResultTypeRepr) { Ctx.Diags.diagnose(AE->getResultExpr()->getLoc(), diag::expected_type_after_arrow); - ResultTypeRepr = new (Ctx) - ErrorTypeRepr(AE->getResultExpr()->getSourceRange()); + ResultTypeRepr = + ErrorTypeRepr::create(Ctx, AE->getResultExpr()->getSourceRange()); } auto NewTypeRepr = new (Ctx) diff --git a/lib/Sema/TypeCheckType.cpp b/lib/Sema/TypeCheckType.cpp index e209d8dd3b30f..d4c6dc7f2ec31 100644 --- a/lib/Sema/TypeCheckType.cpp +++ b/lib/Sema/TypeCheckType.cpp @@ -2431,6 +2431,7 @@ NeverNullType TypeResolver::resolveType(TypeRepr *repr, switch (repr->getKind()) { case TypeReprKind::Error: + cast(repr)->dischargeDiagnostic(getASTContext()); return ErrorType::get(getASTContext()); case TypeReprKind::Attributed: From dbd17c538aec8daa01700d0682213c165ba190f3 Mon Sep 17 00:00:00 2001 From: Kavon Farvardin Date: Mon, 13 Nov 2023 12:05:35 -0800 Subject: [PATCH 2/2] [NCGenerics] allow feature-guarded uses of ~Copyable We need to delay the pre-NCGenerics errors about illegal uses of ~Copyable from Parse until Sema. Otherwise, such uses can't appear within a false --- include/swift/AST/DiagnosticsParse.def | 4 +- include/swift/Parse/Parser.h | 6 +- lib/Parse/ParseDecl.cpp | 91 ++++++++++++-------------- lib/Parse/ParseType.cpp | 15 +++-- test/Parse/inverses_legacy_ifdef.swift | 61 +++++++++++++++++ 5 files changed, 114 insertions(+), 63 deletions(-) create mode 100644 test/Parse/inverses_legacy_ifdef.swift diff --git a/include/swift/AST/DiagnosticsParse.def b/include/swift/AST/DiagnosticsParse.def index 6babd529c9168..84dc1347e6bba 100644 --- a/include/swift/AST/DiagnosticsParse.def +++ b/include/swift/AST/DiagnosticsParse.def @@ -961,8 +961,8 @@ ERROR(cannot_suppress_here,none, ERROR(only_suppress_copyable,none, "can only suppress 'Copyable'", ()) -ERROR(already_suppressed,none, - "duplicate suppression of %0", (Identifier)) +ERROR(already_suppressed_copyable,none, + "duplicate suppression of 'Copyable'", ()) //------------------------------------------------------------------------------ // MARK: Pattern parsing diagnostics diff --git a/include/swift/Parse/Parser.h b/include/swift/Parse/Parser.h index ab771219de90c..ab11749d651d8 100644 --- a/include/swift/Parse/Parser.h +++ b/include/swift/Parse/Parser.h @@ -42,6 +42,7 @@ namespace llvm { namespace swift { class IdentTypeRepr; + class ErrorTypeRepr; class CodeCompletionCallbacks; class DoneParsingCallback; class IDEInspectionCallbacksFactory; @@ -2035,11 +2036,6 @@ class Parser { void performIDEInspectionSecondPassImpl( IDEInspectionDelayedDeclState &info); - /// Returns true if the caller should skip calling `parseType` afterwards. - bool parseLegacyTildeCopyable(SourceLoc *parseTildeCopyable, - ParserStatus &Status, - SourceLoc &TildeCopyableLoc); - //===--------------------------------------------------------------------===// // ASTGen support. diff --git a/lib/Parse/ParseDecl.cpp b/lib/Parse/ParseDecl.cpp index 19a9d47227b93..18e17b9531abf 100644 --- a/lib/Parse/ParseDecl.cpp +++ b/lib/Parse/ParseDecl.cpp @@ -6426,51 +6426,6 @@ static void addMoveOnlyAttrIf(SourceLoc const &parsedTildeCopyable, attrs.add(new(Context) MoveOnlyAttr(/*IsImplicit=*/true)); } -bool Parser::parseLegacyTildeCopyable(SourceLoc *parseTildeCopyable, - ParserStatus &Status, - SourceLoc &TildeCopyableLoc) { - // Is suppression permitted? - if (parseTildeCopyable) { - // Try to find '~' 'Copyable' - // - // We do this knowing that Copyable is not a real type as of now, so we - // can't rely on parseType. - if (Tok.isTilde()) { - const auto &nextTok = peekToken(); // lookahead - if (isIdentifier(nextTok, Context.Id_Copyable.str())) { - auto tildeLoc = consumeToken(); - consumeToken(); // the 'Copyable' token - - if (TildeCopyableLoc) - diagnose(tildeLoc, diag::already_suppressed, Context.Id_Copyable); - else - TildeCopyableLoc = tildeLoc; - - return true; - } else if (nextTok.is(tok::code_complete)) { - consumeToken(); // consume '~' - Status.setHasCodeCompletionAndIsError(); - if (CodeCompletionCallbacks) { - CodeCompletionCallbacks->completeWithoutConstraintType(); - } - consumeToken(tok::code_complete); - } - - // can't suppress whatever is between '~' and ',' or '{'. - diagnose(Tok, diag::only_suppress_copyable); - consumeToken(); - } - - } else if (Tok.isTilde()) { - // a suppression isn't allowed here, so emit an error eat the token to - // prevent further parsing errors. - diagnose(Tok, diag::cannot_suppress_here); - consumeToken(); - } - - return false; -} - /// Parse an inheritance clause. /// /// \verbatim @@ -6546,11 +6501,47 @@ ParserStatus Parser::parseInheritance( continue; } - if (!EnabledNoncopyableGenerics) { - if (parseLegacyTildeCopyable(parseTildeCopyable, - Status, - TildeCopyableLoc)) - continue; + if (!EnabledNoncopyableGenerics && Tok.isTilde()) { + ErrorTypeRepr *error = nullptr; + if (parseTildeCopyable) { + const auto &nextTok = peekToken(); // lookahead + if (isIdentifier(nextTok, Context.Id_Copyable.str())) { + auto tildeLoc = consumeToken(); + consumeToken(); // the 'Copyable' token + + if (TildeCopyableLoc) + Inherited.push_back(InheritedEntry( + ErrorTypeRepr::create(Context, tildeLoc, + diag::already_suppressed_copyable))); + else + TildeCopyableLoc = tildeLoc; + + continue; // success + } + + if (nextTok.is(tok::code_complete)) { + consumeToken(); // consume '~' + Status.setHasCodeCompletionAndIsError(); + if (CodeCompletionCallbacks) { + CodeCompletionCallbacks->completeWithoutConstraintType(); + } + consumeToken(tok::code_complete); + } + + // can't suppress whatever is between '~' and ',' or '{'. + error = ErrorTypeRepr::create(Context, consumeToken(), + diag::only_suppress_copyable); + } else { + // Otherwise, a suppression isn't allowed here unless noncopyable + // generics is enabled, so record a delayed error diagnostic and + // eat the token to prevent further parsing errors. + error = ErrorTypeRepr::create(Context, consumeToken(), + diag::cannot_suppress_here); + } + + // Record the error parsing ~Copyable, but continue on to parseType. + if (error) + Inherited.push_back(InheritedEntry(error)); } auto ParsedTypeResult = parseType(); diff --git a/lib/Parse/ParseType.cpp b/lib/Parse/ParseType.cpp index 4ba7efa36f1a7..819b17f288256 100644 --- a/lib/Parse/ParseType.cpp +++ b/lib/Parse/ParseType.cpp @@ -172,9 +172,6 @@ ParserResult Parser::parseTypeSimple( SourceLoc tildeLoc; if (Tok.isTilde() && !isInSILMode()) { tildeLoc = consumeToken(); - if (!EnabledNoncopyableGenerics) - diagnose(tildeLoc, diag::cannot_suppress_here) - .fixItRemoveChars(tildeLoc, tildeLoc); } switch (Tok.getKind()) { @@ -310,9 +307,15 @@ ParserResult Parser::parseTypeSimple( } // Wrap in an InverseTypeRepr if needed. - if (EnabledNoncopyableGenerics && tildeLoc) { - ty = makeParserResult(ty, - new(Context) InverseTypeRepr(tildeLoc, ty.get())); + if (tildeLoc) { + TypeRepr *repr; + if (EnabledNoncopyableGenerics) + repr = new (Context) InverseTypeRepr(tildeLoc, ty.get()); + else + repr = + ErrorTypeRepr::create(Context, tildeLoc, diag::cannot_suppress_here); + + ty = makeParserResult(ty, repr); } return ty; diff --git a/test/Parse/inverses_legacy_ifdef.swift b/test/Parse/inverses_legacy_ifdef.swift new file mode 100644 index 0000000000000..af6ee6478aa55 --- /dev/null +++ b/test/Parse/inverses_legacy_ifdef.swift @@ -0,0 +1,61 @@ +// RUN: %swift-frontend -typecheck %s +// RUN: %swift-frontend -typecheck %s -DHAVE_NCGENERICS -enable-experimental-feature NoncopyableGenerics +// RUN: %swift-frontend -typecheck %s -verify -DHAVE_NCGENERICS + +// REQUIRES: asserts + +/// This test checks that you can write ~Copyable in places that were illegal +/// in Swift 5.9, as long as those illegal appearances are guarded within +/// a disabled #if block. +/// +/// We previously would emit errors during Parse for those illegal appearances, +/// which would make it impossible to feature-guard new, legal uses of those +/// annotations. Now we delay those diagnostics until Sema. +/// +/// So, the first RUN line here checks that there are no error diagnostics emitted +/// when not providing HAVE_NCGENERICS. Then, we enable that experimental feature to +/// ensure that what's in the #if has no unexpected errors. Finally, we run without that +/// feature to ensure we get the expected errors when the #if-block is allowed to be +/// typechecked. + +struct NC: ~Copyable {} + +#if HAVE_NCGENERICS + +struct Blah: ~Copyable, ~Copyable {} +// expected-error@-1 {{duplicate suppression of 'Copyable'}} + +protocol Compare where Self: ~Copyable { // expected-error {{cannot suppress conformances here}} + func lessThan(_ other: borrowing Self) -> Bool +} + +protocol Sortable: ~Copyable { // expected-error {{cannot suppress conformances here}} + associatedtype Element: ~Copyable, Compare // expected-error {{cannot suppress conformances here}} // expected-note {{protocol requires nested type 'Element'; add nested type 'Element' for conformance}} + + mutating func sort() +} + +extension NC: Compare { // expected-error {{noncopyable struct 'NC' cannot conform to 'Compare'}} + func lessThan(_ other: borrowing Self) -> Bool { false } +} + +extension NC: Sortable { // expected-error {{noncopyable struct 'NC' cannot conform to 'Sortable'}} + // expected-error@-1 {{type 'NC' does not conform to protocol 'Sortable'}} + typealias Element = NC // expected-note {{possibly intended match 'NC.Element' (aka 'NC') does not conform to 'Copyable'}} + + mutating func sort() { } +} + +func f(_ t: inout T) // expected-note {{where 'T.Element' = 'NC.Element'}} + where T: ~Copyable, // expected-error {{cannot suppress conformances here}} + T: Sortable, + T.Element == NC { + t.sort() +} + +do { + var x = NC() + f(&x) // expected-error {{global function 'f' requires the types 'NC.Element' and 'NC' be equivalent}} +} + +#endif