Skip to content

[Name lookup] Enable shadowing for type lookup and of the Swift module. #21370

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
Dec 17, 2018
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: 2 additions & 1 deletion include/swift/AST/Type.h
Original file line number Diff line number Diff line change
Expand Up @@ -647,7 +647,8 @@ namespace llvm {
template<> struct DenseMapInfo<swift::CanType>
: public DenseMapInfo<swift::Type> {
static swift::CanType getEmptyKey() {
return swift::CanType(nullptr);
return swift::CanType(llvm::DenseMapInfo<swift::
TypeBase*>::getEmptyKey());
}
static swift::CanType getTombstoneKey() {
return swift::CanType(llvm::DenseMapInfo<swift::
Expand Down
1 change: 1 addition & 0 deletions include/swift/AST/Types.h
Original file line number Diff line number Diff line change
Expand Up @@ -5346,6 +5346,7 @@ inline CanType CanType::getNominalParent() const {

inline bool CanType::isActuallyCanonicalOrNull() const {
return getPointer() == nullptr ||
getPointer() == llvm::DenseMapInfo<TypeBase *>::getEmptyKey() ||
getPointer() == llvm::DenseMapInfo<TypeBase *>::getTombstoneKey() ||
getPointer()->isCanonical();
}
Expand Down
17 changes: 12 additions & 5 deletions lib/AST/GenericSignature.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include "swift/AST/Types.h"
#include "swift/Basic/STLExtras.h"
#include <functional>
#include "GenericSignatureBuilderImpl.h"

using namespace swift;

Expand Down Expand Up @@ -667,11 +668,17 @@ CanType GenericSignature::getCanonicalTypeInContext(Type type,
!isa<DependentMemberType>(component))
return None;

// Find the equivalence class for this dependent member type.
auto equivClass =
builder.resolveEquivalenceClass(
Type(component),
ArchetypeResolutionKind::CompleteWellFormed);
// Find the equivalence class for this dependent type.
auto resolved = builder.maybeResolveEquivalenceClass(
Type(component),
ArchetypeResolutionKind::CompleteWellFormed,
/*wantExactPotentialArchetype=*/false);
if (!resolved) return None;

if (auto concrete = resolved.getAsConcreteType())
return getCanonicalTypeInContext(concrete, builder);

auto equivClass = resolved.getEquivalenceClass(builder);
if (!equivClass) return None;

if (equivClass->concreteType) {
Expand Down
91 changes: 1 addition & 90 deletions lib/AST/GenericSignatureBuilder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
#include "llvm/Support/raw_ostream.h"
#include "llvm/Support/SaveAndRestore.h"
#include <algorithm>
#include "GenericSignatureBuilderImpl.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: LLVM style says local headers go at the top.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, of course. Committing locally to fix at some point, but I'm not going to re-start CI for that right now.


using namespace swift;
using llvm::DenseMap;
Expand Down Expand Up @@ -1856,96 +1857,6 @@ void EquivalenceClass::addMember(PotentialArchetype *pa) {
}
}

class GenericSignatureBuilder::ResolvedType {
llvm::PointerUnion<PotentialArchetype *, Type> type;
EquivalenceClass *equivClass;

/// For a type that could not be resolved further unless the given
/// equivalence class changes.
ResolvedType(EquivalenceClass *equivClass)
: type(), equivClass(equivClass) { }

public:
/// A specific resolved potential archetype.
ResolvedType(PotentialArchetype *pa)
: type(pa), equivClass(pa->getEquivalenceClassIfPresent()) { }

/// A resolved type within the given equivalence class.
ResolvedType(Type type, EquivalenceClass *equivClass)
: type(type), equivClass(equivClass) {
assert(type->isTypeParameter() == static_cast<bool>(equivClass) &&
"type parameters must have equivalence classes");
}

/// Return an unresolved result, which could be resolved when we
/// learn more information about the given equivalence class.
static ResolvedType forUnresolved(EquivalenceClass *equivClass) {
return ResolvedType(equivClass);
}

/// Return a result for a concrete type.
static ResolvedType forConcrete(Type concreteType) {
return ResolvedType(concreteType, nullptr);
}

/// Determine whether this result was resolved.
explicit operator bool() const { return !type.isNull(); }

/// Retrieve the dependent type.
Type getDependentType(GenericSignatureBuilder &builder) const;

/// Retrieve the concrete type, or a null type if this result doesn't store
/// a concrete type.
Type getAsConcreteType() const {
assert(*this && "Doesn't contain any result");
if (equivClass) return Type();
return type.dyn_cast<Type>();
}

/// Realize a potential archetype for this type parameter.
PotentialArchetype *realizePotentialArchetype(
GenericSignatureBuilder &builder);

/// Retrieve the potential archetype, if already known.
PotentialArchetype *getPotentialArchetypeIfKnown() const {
return type.dyn_cast<PotentialArchetype *>();
}

/// Retrieve the equivalence class into which a resolved type refers.
EquivalenceClass *getEquivalenceClass(
GenericSignatureBuilder &builder) const {
assert(*this && "Only for resolved types");
if (equivClass) return equivClass;

// Create the equivalence class now.
return type.get<PotentialArchetype *>()
->getOrCreateEquivalenceClass(builder);
}

/// Retrieve the equivalence class into which a resolved type refers.
EquivalenceClass *getEquivalenceClassIfPresent() const {
assert(*this && "Only for resolved types");
if (equivClass) return equivClass;

// Create the equivalence class now.
return type.get<PotentialArchetype *>()->getEquivalenceClassIfPresent();
}

/// Retrieve the unresolved result.
EquivalenceClass *getUnresolvedEquivClass() const {
assert(!*this);
return equivClass;
}

/// Return an unresolved type.
///
/// This loses equivalence-class information that could be useful, which
/// is unfortunate.
UnresolvedType getUnresolvedType() const {
return type;
}
};

bool EquivalenceClass::recordConformanceConstraint(
GenericSignatureBuilder &builder,
ResolvedType type,
Expand Down
107 changes: 107 additions & 0 deletions lib/AST/GenericSignatureBuilderImpl.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
//
// GenericSignatureBuilderImpl.h
// Swift
//
// Created by Doug Gregor on 12/17/18.
//

#ifndef SWIFT_AST_GENERIC_SIGNATURE_BUILDER_IMPL_H
#define SWIFT_AST_GENERIC_SIGNATURE_BUILDER_IMPL_H

#include "swift/AST/GenericSignatureBuilder.h"

namespace swift {

class GenericSignatureBuilder::ResolvedType {
llvm::PointerUnion<PotentialArchetype *, Type> type;
EquivalenceClass *equivClass;

/// For a type that could not be resolved further unless the given
/// equivalence class changes.
ResolvedType(EquivalenceClass *equivClass)
: type(), equivClass(equivClass) { }

public:
/// A specific resolved potential archetype.
ResolvedType(PotentialArchetype *pa)
: type(pa), equivClass(pa->getEquivalenceClassIfPresent()) { }

/// A resolved type within the given equivalence class.
ResolvedType(Type type, EquivalenceClass *equivClass)
: type(type), equivClass(equivClass) {
assert(type->isTypeParameter() == static_cast<bool>(equivClass) &&
"type parameters must have equivalence classes");
}

/// Return an unresolved result, which could be resolved when we
/// learn more information about the given equivalence class.
static ResolvedType forUnresolved(EquivalenceClass *equivClass) {
return ResolvedType(equivClass);
}

/// Return a result for a concrete type.
static ResolvedType forConcrete(Type concreteType) {
return ResolvedType(concreteType, nullptr);
}

/// Determine whether this result was resolved.
explicit operator bool() const { return !type.isNull(); }

/// Retrieve the dependent type.
Type getDependentType(GenericSignatureBuilder &builder) const;

/// Retrieve the concrete type, or a null type if this result doesn't store
/// a concrete type.
Type getAsConcreteType() const {
assert(*this && "Doesn't contain any result");
if (equivClass) return Type();
return type.dyn_cast<Type>();
}

/// Realize a potential archetype for this type parameter.
PotentialArchetype *realizePotentialArchetype(
GenericSignatureBuilder &builder);

/// Retrieve the potential archetype, if already known.
PotentialArchetype *getPotentialArchetypeIfKnown() const {
return type.dyn_cast<PotentialArchetype *>();
}

/// Retrieve the equivalence class into which a resolved type refers.
EquivalenceClass *getEquivalenceClass(
GenericSignatureBuilder &builder) const {
assert(*this && "Only for resolved types");
if (equivClass) return equivClass;

// Create the equivalence class now.
return type.get<PotentialArchetype *>()
->getOrCreateEquivalenceClass(builder);
}

/// Retrieve the equivalence class into which a resolved type refers.
EquivalenceClass *getEquivalenceClassIfPresent() const {
assert(*this && "Only for resolved types");
if (equivClass) return equivClass;

// Create the equivalence class now.
return type.get<PotentialArchetype *>()->getEquivalenceClassIfPresent();
}

/// Retrieve the unresolved result.
EquivalenceClass *getUnresolvedEquivClass() const {
assert(!*this);
return equivClass;
}

/// Return an unresolved type.
///
/// This loses equivalence-class information that could be useful, which
/// is unfortunate.
UnresolvedType getUnresolvedType() const {
return type;
}
};

} // end namepsace swift

#endif // SWIFT_AST_GENERIC_SIGNATURE_BUILDER_IMPL_H
64 changes: 47 additions & 17 deletions lib/AST/NameLookup.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,24 @@ static void recordShadowedDeclsAfterSignatureMatch(
break;
}

// Prefer declarations in the any module over those in the standard
// library module.
if (auto swiftModule = ctx.getStdlibModule()) {
if ((firstModule == swiftModule) != (secondModule == swiftModule)) {
// If the second module is the standard library module, the second
// declaration is shadowed by the first.
if (secondModule == swiftModule) {
shadowed.insert(secondDecl);
continue;
}

// Otherwise, the first declaration is shadowed by the second. There is
// no point in continuing to compare the first declaration to others.
shadowed.insert(firstDecl);
break;
}
}

// Prefer declarations in an overlay to similar declarations in
// the Clang module it customizes.
if (firstDecl->hasClangNode() != secondDecl->hasClangNode()) {
Expand Down Expand Up @@ -336,26 +354,33 @@ static void recordShadowedDecls(ArrayRef<ValueDecl *> decls,
}
}

// We need an interface type here.
if (typeResolver)
typeResolver->resolveDeclSignature(decl);
CanType signature;

// If the decl is currently being validated, this is likely a recursive
// reference and we'll want to skip ahead so as to avoid having its type
// attempt to desugar itself.
if (!decl->hasValidSignature())
continue;
if (!isa<TypeDecl>(decl)) {
// We need an interface type here.
if (typeResolver)
typeResolver->resolveDeclSignature(decl);

// FIXME: the canonical type makes a poor signature, because we don't
// canonicalize away default arguments.
auto signature = decl->getInterfaceType()->getCanonicalType();
// If the decl is currently being validated, this is likely a recursive
// reference and we'll want to skip ahead so as to avoid having its type
// attempt to desugar itself.
if (!decl->hasValidSignature())
continue;

// FIXME: The type of a variable or subscript doesn't include
// enough context to distinguish entities from different
// constrained extensions, so use the overload signature's
// type. This is layering a partial fix upon a total hack.
if (auto asd = dyn_cast<AbstractStorageDecl>(decl))
signature = asd->getOverloadSignatureType();
// FIXME: the canonical type makes a poor signature, because we don't
// canonicalize away default arguments.
signature = decl->getInterfaceType()->getCanonicalType();

// FIXME: The type of a variable or subscript doesn't include
// enough context to distinguish entities from different
// constrained extensions, so use the overload signature's
// type. This is layering a partial fix upon a total hack.
if (auto asd = dyn_cast<AbstractStorageDecl>(decl))
signature = asd->getOverloadSignatureType();
} else if (decl->getDeclContext()->isTypeContext()) {
// Do not apply shadowing rules for member types.
continue;
}

// Record this declaration based on its signature.
auto &known = collisions[signature];
Expand Down Expand Up @@ -1169,6 +1194,11 @@ UnqualifiedLookup::UnqualifiedLookup(DeclName Name, DeclContext *DC,
lookupInModule(&M, {}, Name, CurModuleResults, NLKind::UnqualifiedLookup,
resolutionKind, TypeResolver, DC, extraImports);

// Always perform name shadowing for type lookup.
if (options.contains(Flags::TypeLookup)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is iffy—it means that expression context lookup will behave differently than type context lookup. Or does that happen elsewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

It does happen elsewhere. Most unqualified lookup goes through TypeChecker::lookupUnqualified, which (separately) triggers shadowing, so this is bringing things closer together in a duct-tape-and-paper-clip sort of way. There's more refactoring to do, but I'm holding out hope for this to be safe enough for Swift 5.0, hence the narrow applicability (to type lookup, to the Swift module, to non-member types);.

Copy link
Contributor

@jrose-apple jrose-apple Dec 17, 2018

Choose a reason for hiding this comment

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

All right, makes sense. Maybe worth a FIXME, though?

removeShadowedDecls(CurModuleResults, &M);
}

for (auto VD : CurModuleResults)
Results.push_back(LookupResultEntry(VD));

Expand Down
4 changes: 4 additions & 0 deletions test/NameBinding/Inputs/HasResult.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
public enum Result<Value, Error> {
case success(Value)
case failure(Error)
}
7 changes: 7 additions & 0 deletions test/NameBinding/Inputs/MemberTypesInClasses.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
public class RootClass {
}

public class SubClass: RootClass {
public typealias Member = Int
}

15 changes: 15 additions & 0 deletions test/NameBinding/member_type_shadowing.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// RUN: %empty-directory(%t)
// RUN: %target-swift-frontend -emit-module -o %t %S/Inputs/MemberTypesInClasses.swift
// RUN: %target-swift-frontend -typecheck %s -I %t -verify

import MemberTypesInClasses

protocol P {
associatedtype Member
}

extension RootClass: P {
typealias Member = SubClass.Member
}


9 changes: 9 additions & 0 deletions test/NameBinding/stdlib_shadowing.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// RUN: %empty-directory(%t)
// RUN: %target-swift-frontend -emit-module -o %t %S/Inputs/HasResult.swift
// RUN: %target-swift-frontend -typecheck %s -I %t -verify

import HasResult

func foo() -> Result<Int, Error> {
return Result<Int, Error>.success(42)
}