Skip to content

Commit 3127264

Browse files
slavapestovslavapestov
committed
AST: When performing qualified lookup of a member type, filter out non-types earlier
With the previous resolveTypeInContext() patch, a few compiler crashers regressed with this problem, presumably because we were now performing lookups in more contexts than before. This is a class of problems where we would attempt a recursive validation: 1) Generic signature validation begins for type T 2) Name lookup in type context finds a non-type declaration D nested in T 3) Generic signature validation begins for D 4) The outer generic context of D is T, but T doesn't have a generic signature yet The right way to break such cycles is to implement the iterative decl checker design. However when the recursion is via name lookup, we can try to avoid the problem in the first place by not validating non-type declarations if the client requested a type-only lookup. Note that there is a small semantic change here, where programs that were previously rejected as invalid because of name clashes are now valid. It is arguable if we want to allow stuff like this or not: class A { func A(a: A) {} } or class Case {} enum Foo { case Case(Case) } However at the very least, the new behavior is better because it gives us an opportunity to add a diagnostic in the right place later. The old diagnostics were not very good, for example the second example just yields "use of undeclared type 'Case'". In other examples, the undeclared type diagnostic would come up multiple times, or we would generate a cryptic "type 'A' used within its own definition". As far as I understand, this should not change behavior of any existing valid code.
1 parent 5e2f7c9 commit 3127264

21 files changed

+53
-31
lines changed

‎include/swift/AST/LookupKinds.h

+3
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,9 @@ enum NLOptions : unsigned {
6262
/// \see NL_KnownDependencyMask
6363
NL_KnownCascadingDependency = 0x80,
6464

65+
/// This lookup should only return type declarations.
66+
NL_OnlyTypes = 0x100,
67+
6568
/// This lookup is known to not add any additional dependencies to the
6669
/// primary source file.
6770
///

‎include/swift/AST/NameLookup.h

+9-2
Original file line numberDiff line numberDiff line change
@@ -194,11 +194,18 @@ class NamedDeclConsumer : public VisibleDeclConsumer {
194194
public:
195195
DeclName name;
196196
SmallVectorImpl<UnqualifiedLookupResult> &results;
197+
bool isTypeLookup;
198+
197199
NamedDeclConsumer(DeclName name,
198-
SmallVectorImpl<UnqualifiedLookupResult> &results)
199-
: name(name), results(results) {}
200+
SmallVectorImpl<UnqualifiedLookupResult> &results,
201+
bool isTypeLookup)
202+
: name(name), results(results), isTypeLookup(isTypeLookup) {}
200203

201204
virtual void foundDecl(ValueDecl *VD, DeclVisibilityKind Reason) override {
205+
// Give clients an opportunity to filter out non-type declarations early,
206+
// to avoid circular validation.
207+
if (isTypeLookup && !isa<TypeDecl>(VD))
208+
return;
202209
if (VD->getFullName().matchesRef(name))
203210
results.push_back(UnqualifiedLookupResult(VD));
204211
}

‎lib/AST/NameLookup.cpp

+8-1
Original file line numberDiff line numberDiff line change
@@ -385,7 +385,7 @@ UnqualifiedLookup::UnqualifiedLookup(DeclName Name, DeclContext *DC,
385385
const SourceManager &SM = Ctx.SourceMgr;
386386
DebuggerClient *DebugClient = M.getDebugClient();
387387

388-
NamedDeclConsumer Consumer(Name, Results);
388+
NamedDeclConsumer Consumer(Name, Results, IsTypeLookup);
389389

390390
Optional<bool> isCascadingUse;
391391
if (IsKnownNonCascading)
@@ -530,6 +530,8 @@ UnqualifiedLookup::UnqualifiedLookup(DeclName Name, DeclContext *DC,
530530

531531
if (AllowProtocolMembers)
532532
options |= NL_ProtocolMembers;
533+
if (IsTypeLookup)
534+
options |= NL_OnlyTypes;
533535

534536
if (!ExtendedType)
535537
ExtendedType = ErrorType::get(Ctx);
@@ -1262,6 +1264,11 @@ bool DeclContext::lookupQualified(Type type,
12621264
// Look for results within the current nominal type and its extensions.
12631265
bool currentIsProtocol = isa<ProtocolDecl>(current);
12641266
for (auto decl : current->lookupDirect(member)) {
1267+
// If we're performing a type lookup, don't even attempt to validate
1268+
// the decl if its not a type.
1269+
if ((options & NL_OnlyTypes) && !isa<TypeDecl>(decl))
1270+
continue;
1271+
12651272
// Resolve the declaration signature when we find the
12661273
// declaration.
12671274
if (typeResolver && !decl->isBeingTypeChecked()) {

‎lib/Sema/TypeCheckNameLookup.cpp

+5-3
Original file line numberDiff line numberDiff line change
@@ -104,9 +104,9 @@ namespace {
104104
/// \param foundInType The type through which we found the
105105
/// declaration.
106106
void add(ValueDecl *found, ValueDecl *base, Type foundInType) {
107-
// If we only want types and we didn't get one, bail out.
108-
if (Options.contains(NameLookupFlags::OnlyTypes) && !isa<TypeDecl>(found))
109-
return;
107+
// If we only want types, AST name lookup should not yield anything else.
108+
assert(!Options.contains(NameLookupFlags::OnlyTypes) ||
109+
isa<TypeDecl>(found));
110110

111111
ConformanceCheckOptions conformanceOptions;
112112
if (Options.contains(NameLookupFlags::KnownPrivate))
@@ -245,6 +245,8 @@ LookupResult TypeChecker::lookupMember(DeclContext *dc,
245245
subOptions |= NL_DynamicLookup;
246246
if (options.contains(NameLookupFlags::IgnoreAccessibility))
247247
subOptions |= NL_IgnoreAccessibility;
248+
if (options.contains(NameLookupFlags::OnlyTypes))
249+
subOptions |= NL_OnlyTypes;
248250

249251
// Dig out the type that we'll actually be looking into, and determine
250252
// whether it is a nominal type.

‎test/Sema/circular_decl_checking.swift

+6-6
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
// RUN: %target-parse-verify-swift
22

33
class HasFunc {
4-
func HasFunc(_: HasFunc) { // expected-error {{use of undeclared type 'HasFunc'}}
4+
func HasFunc(_: HasFunc) {
55
}
6-
func HasFunc() -> HasFunc { // expected-error {{use of undeclared type 'HasFunc'}}
6+
func HasFunc() -> HasFunc {
77
return HasFunc()
88
}
99
func SomethingElse(_: SomethingElse) { // expected-error {{use of undeclared type 'SomethingElse'}}
@@ -24,17 +24,17 @@ class HasGenericFunc {
2424
}
2525

2626
class HasProp {
27-
var HasProp: HasProp { // expected-error {{'HasProp' used within its own type}} expected-error 2 {{use of undeclared type 'HasProp'}}
28-
return HasProp()
27+
var HasProp: HasProp {
28+
return HasProp() // expected-error {{cannot call value of non-function type 'HasProp'}}
2929
}
30-
var SomethingElse: SomethingElse? { // expected-error {{'SomethingElse' used within its own type}} expected-error 2 {{use of undeclared type 'SomethingElse'}}
30+
var SomethingElse: SomethingElse? { // expected-error 2 {{use of undeclared type 'SomethingElse'}}
3131
return nil
3232
}
3333
}
3434

3535
protocol SomeProtocol {}
3636
protocol ReferenceSomeProtocol {
37-
var SomeProtocol: SomeProtocol { get } // expected-error {{'SomeProtocol' used within its own type}} expected-error 2 {{use of undeclared type 'SomeProtocol'}}
37+
var SomeProtocol: SomeProtocol { get }
3838
}
3939

4040
func TopLevelFunc(x: TopLevelFunc) -> TopLevelFunc { return x } // expected-error {{use of undeclared type 'TopLevelFunc'}}'

‎test/decl/init/basic_init.swift

+1-1
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ class Foo {
55
}
66

77
class C {
8-
var triangle : triangle // expected-error {{'triangle' used within its own type}} expected-error{{use of undeclared type 'triangle'}}
8+
var triangle : triangle // expected-error{{use of undeclared type 'triangle'}}
99

1010
init() {}
1111
}

‎test/decl/typealias/typealias.swift

+4-1
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,9 @@
33
typealias rgb = Int32 // expected-note {{declared here}}
44
var rgb : rgb? // expected-error {{invalid redeclaration of 'rgb'}}
55

6+
// This used to produce a diagnostic about 'rgba' being used in its own
7+
// type, but arguably that is incorrect, since we are referencing a
8+
// different 'rgba'.
69
struct Color {
710
var rgba : rgba? { // expected-error {{'rgba' used within its own type}}
811
return nil
@@ -12,7 +15,7 @@ struct Color {
1215
}
1316

1417
struct Color2 {
15-
let rgba : rgba? // expected-error {{'rgba' used within its own type}}
18+
let rgba : rgba?
1619

1720
struct rgba {}
1821
}

‎validation-test/IDE/crashers/015-swift-typechecker-lookupunqualified.swift

-3
This file was deleted.

‎validation-test/IDE/crashers/067-swift-constraints-constraintsystem-resolveoverload.swift

-4
This file was deleted.

‎validation-test/IDE/crashers_fixed/015-swift-typechecker-lookupunqualified.swift

+3
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
// RUN: %target-swift-ide-test -code-completion -code-completion-token=A -source-filename=%s
2+
// REQUIRES: asserts
3+
T{struct B{let h=protocol P{struct B<T where h:e{var f=B#^A^#

‎validation-test/IDE/crashers_fixed/067-swift-constraints-constraintsystem-resolveoverload.swift

+4
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
// RUN: %target-swift-ide-test -code-completion -code-completion-token=A -source-filename=%s
2+
// REQUIRES: asserts
3+
struct d{func b
4+
let g=b<a let a=enum b<j where g:p:#^A^#

‎validation-test/compiler_crashers/27131-isvalidoverload.swift ‎validation-test/compiler_crashers_fixed/27131-isvalidoverload.swift

+1-1
Original file line numberDiff line numberDiff line change
@@ -5,5 +5,5 @@
55
// See http://swift.org/LICENSE.txt for license information
66
// See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
77

8-
// RUN: not --crash %target-swift-frontend %s -parse
8+
// RUN: not %target-swift-frontend %s -parse
99
struct A{class d:a{protocol a{func<}}let a=S<

‎validation-test/compiler_crashers/27636-swift-typechecker-resolvetypeincontext.swift ‎validation-test/compiler_crashers_fixed/27636-swift-typechecker-resolvetypeincontext.swift

+1-1
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
// See http://swift.org/LICENSE.txt for license information
66
// See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
77

8-
// RUN: not --crash %target-swift-frontend %s -parse
8+
// RUN: not %target-swift-frontend %s -parse
99
// ASAN Output: stack-overflow on address 0x7ffdcd2b1fd0 (pc 0x0000008ecf9e bp 0x7ffdcd2b2810 sp 0x7ffdcd2b1fc0 T0)
1010
enum A
1111
protocol A{

‎validation-test/compiler_crashers/27832-swift-typechecker-resolvetypeincontext.swift ‎validation-test/compiler_crashers_fixed/27832-swift-typechecker-resolvetypeincontext.swift

+1-1
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
// See http://swift.org/LICENSE.txt for license information
66
// See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
77

8-
// RUN: not --crash %target-swift-frontend %s -parse
8+
// RUN: not %target-swift-frontend %s -parse
99
// ASAN Output: stack-overflow on address 0x7ffd2c334fb0 (pc 0x0000008ecf9e bp 0x7ffd2c3357f0 sp 0x7ffd2c334fa0 T0)
1010
enum A
1111
protocol A{

‎validation-test/compiler_crashers/28195-swift-constraints-constraintsystem-resolveoverload.swift ‎validation-test/compiler_crashers_fixed/28195-swift-constraints-constraintsystem-resolveoverload.swift

+1-1
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
// See http://swift.org/LICENSE.txt for license information
66
// See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
77

8-
// RUN: not --crash %target-swift-frontend %s -parse
8+
// RUN: not %target-swift-frontend %s -parse
99
// REQUIRES: asserts
1010
enum a{
1111
func b

‎validation-test/compiler_crashers/28227-swift-typechecker-gettypeofrvalue.swift ‎validation-test/compiler_crashers_fixed/28227-swift-typechecker-gettypeofrvalue.swift

+1-1
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,6 @@
55
// See http://swift.org/LICENSE.txt for license information
66
// See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
77

8-
// RUN: not --crash %target-swift-frontend %s -parse
8+
// RUN: not %target-swift-frontend %s -parse
99
// REQUIRES: asserts
1010
struct c{struct A:a{var d}let a=A{}protocol A

‎validation-test/compiler_crashers/28251-swift-typechecker-addimplicitconstructors.swift ‎validation-test/compiler_crashers_fixed/28251-swift-typechecker-addimplicitconstructors.swift

+1-1
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
// See http://swift.org/LICENSE.txt for license information
66
// See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
77

8-
// RUN: not --crash %target-swift-frontend %s -parse
8+
// RUN: not %target-swift-frontend %s -parse
99
// REQUIRES: asserts
1010
enum w{class d:a
1111
var d=[[]a

‎validation-test/compiler_crashers/28253-swift-constraints-constraintsystem-matchdeepequalitytypes.swift ‎validation-test/compiler_crashers_fixed/28253-swift-constraints-constraintsystem-matchdeepequalitytypes.swift

+1-1
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
// See http://swift.org/LICENSE.txt for license information
66
// See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
77

8-
// RUN: not --crash %target-swift-frontend %s -parse
8+
// RUN: not %target-swift-frontend %s -parse
99
// REQUIRES: asserts
1010
class A{let h=B<
1111
protocol A{

‎validation-test/compiler_crashers/28298-swift-namealiastype-getsinglydesugaredtype.swift ‎validation-test/compiler_crashers_fixed/28298-swift-namealiastype-getsinglydesugaredtype.swift

+1-1
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
// See http://swift.org/LICENSE.txt for license information
66
// See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
77

8-
// RUN: not --crash %target-swift-frontend %s -parse
8+
// RUN: not %target-swift-frontend %s -parse
99
// REQUIRES: asserts
1010
class A{typealias e:a
1111
let a=c

‎validation-test/compiler_crashers/28300-swift-type-transform.swift ‎validation-test/compiler_crashers_fixed/28300-swift-type-transform.swift

+1-1
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
// See http://swift.org/LICENSE.txt for license information
66
// See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
77

8-
// RUN: not --crash %target-swift-frontend %s -parse
8+
// RUN: not %target-swift-frontend %s -parse
99
// REQUIRES: asserts
1010
class S<T{
1111
class A{

‎validation-test/compiler_crashers/28309-swift-typechecker-addimplicitconstructors.swift ‎validation-test/compiler_crashers_fixed/28309-swift-typechecker-addimplicitconstructors.swift

+1-1
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
// See http://swift.org/LICENSE.txt for license information
66
// See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
77

8-
// RUN: not --crash %target-swift-frontend %s -parse
8+
// RUN: not %target-swift-frontend %s -parse
99
// REQUIRES: asserts
1010
class a
1111
class A:a{

0 commit comments

Comments
 (0)