Skip to content

[CSGen] Turn invalid decls into holes #34584

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Nov 5, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions include/swift/AST/DiagnosticsSema.def
Original file line number Diff line number Diff line change
Expand Up @@ -3726,6 +3726,9 @@ ERROR(capture_across_type_decl,none,
"%0 declaration cannot close over value %1 defined in outer scope",
(DescriptiveDeclKind, Identifier))

ERROR(reference_to_invalid_decl,none,
"cannot reference invalid declaration %0", (DeclName))

//------------------------------------------------------------------------------
// MARK: Type Check Statements
//------------------------------------------------------------------------------
Expand Down
23 changes: 23 additions & 0 deletions include/swift/Sema/CSFix.h
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,10 @@ enum class FixKind : uint8_t {

/// Resolve type of `nil` by providing a contextual type.
SpecifyContextualTypeForNil,

/// Allow expressions to reference invalid declarations by turning
/// them into holes.
AllowRefToInvalidDecl,
};

class ConstraintFix {
Expand Down Expand Up @@ -2055,6 +2059,25 @@ class SpecifyContextualTypeForNil final : public ConstraintFix {
ConstraintLocator * locator);
};

class AllowRefToInvalidDecl final : public ConstraintFix {
AllowRefToInvalidDecl(ConstraintSystem &cs, ConstraintLocator *locator)
: ConstraintFix(cs, FixKind::AllowRefToInvalidDecl, locator) {}

public:
std::string getName() const override {
return "ignore invalid declaration reference";
}

bool diagnose(const Solution &solution, bool asNote = false) const override;

bool diagnoseForAmbiguity(CommonFixesArray commonFixes) const override {
return diagnose(*commonFixes.front().first);
}

static AllowRefToInvalidDecl *create(ConstraintSystem &cs,
ConstraintLocator *locator);
};

} // end namespace constraints
} // end namespace swift

Expand Down
18 changes: 18 additions & 0 deletions lib/Sema/CSDiagnostics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7002,3 +7002,21 @@ bool MissingContextualTypeForNil::diagnoseAsError() {
emitDiagnostic(diag::unresolved_nil_literal);
return true;
}

bool ReferenceToInvalidDeclaration::diagnoseAsError() {
auto *decl = castToExpr<DeclRefExpr>(getAnchor())->getDecl();
assert(decl);

auto &DE = getASTContext().Diags;
// This problem should have been already diagnosed during
// validation of the declaration.
if (DE.hadAnyError())
return true;

// If no errors have been emitted yet, let's emit one
// about reference to an invalid declaration.

emitDiagnostic(diag::reference_to_invalid_decl, decl->getName());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When would the solver actually emit this error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In cases when actual problem with declaration hasn't been diagnosed due to a bug (hopefully never).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about adding bit about asking to report it just like we do with actual fallback diagnostic.

emitDiagnosticAt(decl, diag::decl_declared_here, decl->getName());
return true;
}
15 changes: 15 additions & 0 deletions lib/Sema/CSDiagnostics.h
Original file line number Diff line number Diff line change
Expand Up @@ -2281,6 +2281,21 @@ class MissingContextualTypeForNil final : public FailureDiagnostic {
bool diagnoseAsError() override;
};

/// Diagnostic situations where AST node references an invalid declaration.
///
/// \code
/// let foo = doesntExist // or something invalid
/// foo(42)
/// \endcode
class ReferenceToInvalidDeclaration final : public FailureDiagnostic {
public:
ReferenceToInvalidDeclaration(const Solution &solution,
ConstraintLocator *locator)
: FailureDiagnostic(solution, locator) {}

bool diagnoseAsError() override;
};

} // end namespace constraints
} // end namespace swift

Expand Down
12 changes: 12 additions & 0 deletions lib/Sema/CSFix.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1615,3 +1615,15 @@ SpecifyContextualTypeForNil::create(ConstraintSystem &cs,
ConstraintLocator *locator) {
return new (cs.getAllocator()) SpecifyContextualTypeForNil(cs, locator);
}

bool AllowRefToInvalidDecl::diagnose(const Solution &solution,
bool asNote) const {
ReferenceToInvalidDeclaration failure(solution, getLocator());
return failure.diagnose(asNote);
}

AllowRefToInvalidDecl *
AllowRefToInvalidDecl::create(ConstraintSystem &cs,
ConstraintLocator *locator) {
return new (cs.getAllocator()) AllowRefToInvalidDecl(cs, locator);
}
19 changes: 10 additions & 9 deletions lib/Sema/CSGen.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1227,9 +1227,11 @@ namespace {
if (knownType) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just above this, within the CS.getVarType(VD) call it's getting the type of VD and replacing any ErrorTypes it contains with holes when CS.isForCodeCompletion is true. Does this change make that logic redundant?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for code completion we'd want to preserve as much structure as possible so replacing everything with a single hole might not be the best way... I was thinking more of a second case where E->getDecl()->isInvalid() is true.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is, could we remove it and make the below behave the same way, preserving any wrapping types? (e.g. [ErrorType] becomes [<hole>] rather than just <hole>?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't see your reply before I sent that, sorry.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I could try to unify logic here to preserve some more structure for diagnostics, I'll give it a try!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into this some more and it looks like for solver and diagnostics in particular it make more sense to keep invalid declarations represented as type variables so they could assume any time from context instead otherwise we'd risk diagnosing issues which are effectively already diagnosed while validating declarations...

// If the known type has an error, bail out.
if (knownType->hasError()) {
auto *hole = CS.createTypeVariable(locator, TVO_CanBindToHole);
(void)CS.recordFix(AllowRefToInvalidDecl::create(CS, locator));
if (!CS.hasType(E))
CS.setType(E, knownType);
return nullptr;
CS.setType(E, hole);
return hole;
}

if (!knownType->hasHole()) {
Expand Down Expand Up @@ -1258,14 +1260,13 @@ namespace {
}
}

// If we're referring to an invalid declaration, don't type-check.
//
// FIXME: If the decl is in error, we get no information from this.
// We may, alternatively, want to use a type variable in that case,
// and possibly infer the type of the variable that way.
// If declaration is invalid, let's turn it into a potential hole
// and keep generating constraints.
if (!knownType && E->getDecl()->isInvalid()) {
CS.setType(E, E->getDecl()->getInterfaceType());
return nullptr;
auto *hole = CS.createTypeVariable(locator, TVO_CanBindToHole);
(void)CS.recordFix(AllowRefToInvalidDecl::create(CS, locator));
CS.setType(E, hole);
return hole;
}

// Create an overload choice referencing this declaration and immediately
Expand Down
3 changes: 2 additions & 1 deletion lib/Sema/CSSimplify.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10144,7 +10144,8 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
case FixKind::SpecifyLabelToAssociateTrailingClosure:
case FixKind::AllowKeyPathWithoutComponents:
case FixKind::IgnoreInvalidResultBuilderBody:
case FixKind::SpecifyContextualTypeForNil: {
case FixKind::SpecifyContextualTypeForNil:
case FixKind::AllowRefToInvalidDecl: {
return recordFix(fix) ? SolutionKind::Error : SolutionKind::Solved;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
// expected-error@-1 {{cannot find 'CGRect' in scope}}

let (r, s) = square.divided(atDistance: 50, from: .minXEdge)
// expected-error@-1 {{type of expression is ambiguous without more context}}
#endif

#if canImport(MixedWithHeader)
Expand Down
4 changes: 2 additions & 2 deletions test/ClangImporter/cf.swift
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
import CoreCooling
import CFAndObjC

func assertUnmanaged<T>(_ t: Unmanaged<T>) {}
func assertUnmanaged<T>(_ t: Unmanaged<T>) {} // expected-note {{in call to function 'assertUnmanaged'}}
func assertManaged<T: AnyObject>(_ t: T) {}

func test0(_ fridge: CCRefrigerator) {
Expand All @@ -15,7 +15,7 @@ func test0(_ fridge: CCRefrigerator) {
func test1(_ power: Unmanaged<CCPowerSupply>) {
assertUnmanaged(power)
let fridge = CCRefrigeratorCreate(power) // expected-error {{cannot convert value of type 'Unmanaged<CCPowerSupply>' to expected argument type 'CCPowerSupply?'}}
assertUnmanaged(fridge)
assertUnmanaged(fridge) // expected-error {{generic parameter 'T' could not be inferred}}
}

func test2() {
Expand Down
4 changes: 2 additions & 2 deletions test/NameLookup/name_lookup.swift
Original file line number Diff line number Diff line change
Expand Up @@ -650,6 +650,6 @@ struct PatternBindingWithTwoVars3 { var x = y, y = x }

// https://bugs.swift.org/browse/SR-9015
func sr9015() {
let closure1 = { closure2() } // expected-error {{circular reference}} expected-note {{through reference here}} expected-note {{through reference here}} expected-error {{unable to infer closure}}
let closure2 = { closure1() } // expected-note {{through reference here}} expected-note {{through reference here}} expected-note {{through reference here}} expected-error {{unable to infer closure}}
let closure1 = { closure2() } // expected-error {{circular reference}} expected-note {{through reference here}} expected-note {{through reference here}}
let closure2 = { closure1() } // expected-note {{through reference here}} expected-note {{through reference here}} expected-note {{through reference here}}
}
12 changes: 6 additions & 6 deletions test/attr/attr_noescape.swift
Original file line number Diff line number Diff line change
Expand Up @@ -184,8 +184,8 @@ class SomeClass {
// Implicit conversions (in this case to @convention(block)) are ok.
@_silgen_name("whatever")
func takeNoEscapeAsObjCBlock(_: @noescape @convention(block) () -> Void) // expected-error{{unknown attribute 'noescape'}}
func takeNoEscapeTest2(_ fn : @noescape () -> ()) { // expected-error{{unknown attribute 'noescape'}}
takeNoEscapeAsObjCBlock(fn)
func takeNoEscapeTest2(_ fn : @noescape () -> ()) { // expected-error{{unknown attribute 'noescape'}} expected-note {{parameter 'fn' is implicitly non-escaping}}
takeNoEscapeAsObjCBlock(fn) // expected-error {{passing non-escaping parameter 'fn' to function expecting an @escaping closure}}
}

// Autoclosure implies noescape..
Expand Down Expand Up @@ -290,14 +290,14 @@ typealias CompletionHandler = (_ success: Bool) -> ()

var escape : CompletionHandlerNE
var escapeOther : CompletionHandler
func doThing1(_ completion: (_ success: Bool) -> ()) {
escape = completion
func doThing1(_ completion: (_ success: Bool) -> ()) { // expected-note {{parameter 'completion' is implicitly non-escaping}}
escape = completion // expected-error {{assigning non-escaping parameter 'completion' to an @escaping closure}}
}
func doThing2(_ completion: CompletionHandlerNE) {
escape = completion
}
func doThing3(_ completion: CompletionHandler) {
escape = completion
func doThing3(_ completion: CompletionHandler) { // expected-note {{parameter 'completion' is implicitly non-escaping}}
escape = completion // expected-error {{assigning non-escaping parameter 'completion' to an @escaping closure}}
}
func doThing4(_ completion: @escaping CompletionHandler) {
escapeOther = completion
Expand Down
1 change: 0 additions & 1 deletion test/expr/closure/closures.swift
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,6 @@ assert(f0(1) == 1)
var selfRef = { selfRef() }
// expected-note@-1 2{{through reference here}}
// expected-error@-2 {{circular reference}}
// expected-error@-3 {{unable to infer closure type in the current context}}

var nestedSelfRef = {
var recursive = { nestedSelfRef() }
Expand Down
15 changes: 15 additions & 0 deletions validation-test/Sema/SwiftUI/rdar70880670.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// RUN: %target-typecheck-verify-swift -target x86_64-apple-macosx10.15 -swift-version 5
// REQUIRES: objc_interop
// REQUIRES: OS=macosx

import SwiftUI

var foo = doesntExist // expected-error {{cannot find 'doesntExist' in scope}}

struct ContentView: View {
var body: some View {
VStack {
Text(verbatim: foo)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ func checksum(value: UInt16) -> UInt16 {
var checksum = (((value >> 2) ^ (value >> 8) ^ (value >> 12) ^ (value >> 14)) & 0x01) << 1
// expected-error@-1 {{the compiler is unable to type-check this expression in reasonable time}}
checksum |= (((value) ^ (value >> UInt16(4)) ^ (value >> UInt16(6)) ^ (value >> UInt16(10))) & 0x01)
// expected-error@-1 {{the compiler is unable to type-check this expression in reasonable time}}
checksum ^= 0x02
return checksum
}
Expand Down