Skip to content

[Sema][GSB] Warn about redundant ': Any' conformances & constraints #17425

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.

Sign up for GitHub

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

Closed
wants to merge 7 commits into from
Closed

[Sema][GSB] Warn about redundant ': Any' conformances & constraints #17425

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
331e9e8
[Sema] Fix implementation of getStartLocOfInheritanceClause
hamishknight Jun 11, 2018
ef36f87
[Sema] Warn about stated conformances to 'Any'
hamishknight Jun 11, 2018
4ae85ac
[GSB] Use Type instead of ProtocolDecl in a couple of diagnostics [NFC]
hamishknight Jun 15, 2018
94e4e38
[GSB] Warn about redundant ': Any' type constraints
hamishknight Jun 15, 2018
099430d
[Parse][GSB] Add fix-it to remove redundant ': Any' constraints
hamishknight Jun 15, 2018
8d0dadd
[Parse][Sema] Avoid overlapping fix-it removal ranges for redundant '…
hamishknight Jun 29, 2018
d5d6586
[Sema] Emit a duplicate inheritance error for duplicate 'Any' conform…
hamishknight Jul 2, 2018
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
78 changes: 57 additions & 21 deletions include/swift/AST/Decl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1012,6 +1012,12 @@ enum class RequirementReprKind : unsigned {
/// \c GenericParamList assumes these are POD-like.
class RequirementRepr {
SourceLoc SeparatorLoc;

/// This range includes everything that needs to be removed to keep the
/// program syntactically valid (e.g. where and commas), unlike just from
/// the start of \c FirstType to the end of \c SecondType.
SourceRange RemovalRange;

RequirementReprKind Kind : 2;
bool Invalid : 1;
TypeLoc FirstType;
Expand All @@ -1027,15 +1033,17 @@ class RequirementRepr {
/// for the generated interface.
StringRef AsWrittenString;

RequirementRepr(SourceLoc SeparatorLoc, RequirementReprKind Kind,
TypeLoc FirstType, TypeLoc SecondType)
: SeparatorLoc(SeparatorLoc), Kind(Kind), Invalid(false),
FirstType(FirstType), SecondType(SecondType) { }
RequirementRepr(SourceLoc SeparatorLoc, SourceRange RemovalRange,
RequirementReprKind Kind, TypeLoc FirstType,
TypeLoc SecondType)
: SeparatorLoc(SeparatorLoc), RemovalRange(RemovalRange), Kind(Kind),
Invalid(false), FirstType(FirstType), SecondType(SecondType) {}

RequirementRepr(SourceLoc SeparatorLoc, RequirementReprKind Kind,
TypeLoc FirstType, LayoutConstraintLoc SecondLayout)
: SeparatorLoc(SeparatorLoc), Kind(Kind), Invalid(false),
FirstType(FirstType), SecondLayout(SecondLayout) { }
RequirementRepr(SourceLoc SeparatorLoc, SourceRange RemovalRange,
RequirementReprKind Kind, TypeLoc FirstType,
LayoutConstraintLoc SecondLayout)
: SeparatorLoc(SeparatorLoc), RemovalRange(RemovalRange), Kind(Kind),
Invalid(false), FirstType(FirstType), SecondLayout(SecondLayout) {}

void printImpl(ASTPrinter &OS, bool AsWritten) const;

Expand All @@ -1048,10 +1056,15 @@ class RequirementRepr {
/// this requirement was implied.
/// \param Constraint The protocol or protocol composition to which the
/// subject must conform, or superclass from which the subject must inherit.
static RequirementRepr getTypeConstraint(TypeLoc Subject,
SourceLoc ColonLoc,
TypeLoc Constraint) {
return { ColonLoc, RequirementReprKind::TypeConstraint, Subject, Constraint };
/// \param RemovalRange A source range suitable for removing the requirement
/// from the associated where clause, including everything that needs to be
/// removed to keep the program syntactically valid (e.g. the 'where' keyword
/// and commas).
static RequirementRepr getTypeConstraint(TypeLoc Subject, SourceLoc ColonLoc,
TypeLoc Constraint,
SourceRange RemovalRange) {
return {ColonLoc, RemovalRange, RequirementReprKind::TypeConstraint,
Subject, Constraint};
}

/// \brief Construct a new same-type requirement.
Expand All @@ -1060,25 +1073,34 @@ class RequirementRepr {
/// \param EqualLoc The location of the '==' in the same-type constraint, or
/// an invalid location if this requirement was implied.
/// \param SecondType The second type.
static RequirementRepr getSameType(TypeLoc FirstType,
SourceLoc EqualLoc,
TypeLoc SecondType) {
return { EqualLoc, RequirementReprKind::SameType, FirstType, SecondType };
/// \param RemovalRange A source range suitable for removing the requirement
/// from the associated where clause, including everything that needs to be
/// removed to keep the program syntactically valid (e.g. the 'where' keyword
/// and commas).
static RequirementRepr getSameType(TypeLoc FirstType, SourceLoc EqualLoc,
TypeLoc SecondType,
SourceRange RemovalRange) {
return {EqualLoc, RemovalRange, RequirementReprKind::SameType, FirstType,
SecondType};
}

/// \brief Construct a new layout-constraint requirement.
///
/// \param Subject The type that must conform to the given layout
/// requirement.
/// \param Subject The type that must conform to the given layout requirement.
/// \param ColonLoc The location of the ':', or an invalid location if
/// this requirement was implied.
/// \param Layout The layout requirement to which the
/// subject must conform.
/// \param RemovalRange A source range suitable for removing the requirement
/// from the associated where clause, including everything that needs to be
/// removed to keep the program syntactically valid (e.g. the 'where' keyword
/// and commas).
static RequirementRepr getLayoutConstraint(TypeLoc Subject,
SourceLoc ColonLoc,
LayoutConstraintLoc Layout) {
return {ColonLoc, RequirementReprKind::LayoutConstraint, Subject,
Layout};
LayoutConstraintLoc Layout,
SourceRange RemovalRange) {
return {ColonLoc, RemovalRange, RequirementReprKind::LayoutConstraint,
Subject, Layout};
}

/// \brief Determine the kind of requirement
Expand Down Expand Up @@ -1161,6 +1183,20 @@ class RequirementRepr {
return SeparatorLoc;
}

/// Retrieve the source range suitable for removing the requirement, if any.
/// Unlike \c getSourceRange(), this range includes everything that needs to
/// be removed to keep the program syntactically valid (e.g. the 'where'
/// keyword and commas).
SourceRange getRemovalRange() const { return RemovalRange; }

/// Set the source range suitable for removing the requirement, if any. Unlike
/// \c getSourceRange(), this range includes everything that needs to be
/// removed to keep the program syntactically valid (e.g. the 'where' keyword
/// and commas).
void setRemovalRange(SourceRange removalRange) {
RemovalRange = removalRange;
}

/// \brief Retrieve the first type of a same-type requirement.
Type getFirstType() const {
assert(getKind() == RequirementReprKind::SameType);
Expand Down
9 changes: 7 additions & 2 deletions include/swift/AST/DiagnosticsSema.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1834,6 +1834,11 @@ WARNING(redundant_conformance_adhoc_conditional,none,
NOTE(redundant_conformance_witness_ignored,none,
"%0 %1 will not be used to satisfy the conformance to %2",
(DescriptiveDeclKind, DeclName, DeclName))
WARNING(redundant_conformance_warning,none,
"redundant conformance of %0 to %1", (Type, Type))
NOTE(all_types_implicitly_conform_to,none,
"all types implicitly conform to %0", (Type))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An alternative phrasing for the note would be "all types are subtypes of %0", which is more technically correct for Any as it's not actually a protocol you can conform to (just an empty protocol composition) – but I feel the current wording is more user friendly. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fair to say that one conforms to a protocol composition (e.g. we say that people conform to typealias Codable = Encodable & Decodable), even an empty one. I vote on keeping what you've written.


// "Near matches"
WARNING(req_near_match,none,
Expand Down Expand Up @@ -1908,11 +1913,11 @@ ERROR(requires_generic_param_same_type_does_not_conform,none,
ERROR(requires_same_concrete_type,none,
"generic signature requires types %0 and %1 to be the same", (Type, Type))
WARNING(redundant_conformance_constraint,none,
"redundant conformance constraint %0: %1", (Type, ProtocolDecl *))
"redundant conformance constraint %0: %1", (Type, Type))
NOTE(redundant_conformance_here,none,
"conformance constraint %1: %2 %select{written here|implied here|"
"inferred from type here}0",
(unsigned, Type, ProtocolDecl *))
(unsigned, Type, Type))

ERROR(unsupported_recursive_requirements, none,
"requirement involves recursion that is not currently supported", ())
Expand Down
22 changes: 20 additions & 2 deletions include/swift/AST/GenericSignatureBuilder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1276,9 +1276,18 @@ class GenericSignatureBuilder::RequirementSource final
ProtocolDecl *proto,
bool &derivedViaConcrete) const;

/// Retrieve a source location that corresponds to the requirement.
/// Retrieve a source location that corresponds to the requirement, if any.
SourceLoc getLoc() const;

/// Retrieve the source range for the requirement, if any.
SourceRange getSourceRange() const;

/// Retrieve the source range suitable for removing the requirement, if any.
/// Unlike \c getSourceRange(), this range includes everything that needs to
/// be removed to keep the program syntactically valid (e.g. the 'where'
/// keyword and commas).
SourceRange getRemovalRange() const;

/// Compare two requirement sources to determine which has the more
/// optimal path.
///
Expand Down Expand Up @@ -1449,9 +1458,18 @@ class GenericSignatureBuilder::FloatingRequirementSource {
const RequirementSource *getSource(GenericSignatureBuilder &builder,
Type type) const;

/// Retrieve the source location for this requirement.
/// Retrieve the source location for this requirement, if any.
SourceLoc getLoc() const;

/// Retrieve the source range for this requirement, if any.
SourceRange getSourceRange() const;

/// Retrieve the source range suitable for removing the requirement, if any.
/// Unlike \c getSourceRange(), this range includes everything that needs to
/// be removed to keep the program syntactically valid (e.g. the 'where'
/// keyword and commas).
SourceRange getRemovalRange() const;

/// Whether this is an explicitly-stated requirement.
bool isExplicit() const;

Expand Down
9 changes: 6 additions & 3 deletions lib/AST/Decl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -614,7 +614,8 @@ GenericParamList::clone(DeclContext *dc) const {
reqt = RequirementRepr::getTypeConstraint(
first.clone(ctx),
reqt.getColonLoc(),
second.clone(ctx));
second.clone(ctx),
reqt.getRemovalRange());
break;
}
case RequirementReprKind::SameType: {
Expand All @@ -623,7 +624,8 @@ GenericParamList::clone(DeclContext *dc) const {
reqt = RequirementRepr::getSameType(
first.clone(ctx),
reqt.getEqualLoc(),
second.clone(ctx));
second.clone(ctx),
reqt.getRemovalRange());
break;
}
case RequirementReprKind::LayoutConstraint: {
Expand All @@ -632,7 +634,8 @@ GenericParamList::clone(DeclContext *dc) const {
reqt = RequirementRepr::getLayoutConstraint(
first.clone(ctx),
reqt.getColonLoc(),
layout);
layout,
reqt.getRemovalRange());
break;
}
}
Expand Down
Loading