Skip to content

Add warning when comparing a non-optional value to Optional.none #60893

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

Merged
merged 9 commits into from
Sep 12, 2022
Merged

Add warning when comparing a non-optional value to Optional.none #60893

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ded9b78
Warn on redundant comparison to 'Optional.none', like we already do f…
calda Sep 1, 2022
88b65f4
Update tests
calda Sep 1, 2022
2e76562
Combine the two diagnostics
calda Sep 1, 2022
1b224bd
Add a test that verifyies comparing to a .none case that isn't Option…
calda Sep 1, 2022
568549f
Update comment
calda Sep 1, 2022
c2bf0f9
Revert "Combine the two diagnostics"
calda Sep 1, 2022
c3e74e0
Use a single warning, but dynamically specify 'nil' or 'Optional.none'
calda Sep 1, 2022
992a9d3
remove warning test case that doesn't actually compile
calda Sep 1, 2022
d9be4a7
Update check for Optional.none
calda Sep 9, 2022
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
4 changes: 2 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 @@ -4007,8 +4007,8 @@ WARNING(use_of_qq_on_non_optional_value,none,
"left side of nil coalescing operator '?""?' has non-optional type %0, "
"so the right side is never used", (Type))
WARNING(nonoptional_compare_to_nil,none,
"comparing non-optional value of type %0 to 'nil' always returns"
" %select{false|true}1", (Type, bool))
"comparing non-optional value of type %0 to '%select{Optional.none|nil}1' always returns"
" %select{false|true}2", (Type, bool, bool))
WARNING(optional_check_nonoptional,none,
"non-optional expression of type %0 used in a check for optionals",
(Type))
Expand Down
23 changes: 12 additions & 11 deletions lib/Sema/MiscDiagnostics.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1285,9 +1285,12 @@ static void diagSyntacticUseRestrictions(const Expr *E, const DeclContext *DC,
/// Return true if this is a 'nil' literal. This looks
/// like this if the type is Optional<T>:
///
/// (dot_syntax_call_expr implicit type='Int?'
/// (declref_expr implicit decl=Optional.none)
/// (type_expr type=Int?))
/// (dot_syntax_call_expr type='String?'
/// (declref_expr type='(Optional<String>.Type) -> Optional<String>'
/// decl=Swift.(file).Optional.none function_ref=unapplied)
/// (argument_list implicit
/// (argument
/// (type_expr implicit type='String?.Type' typerepr='String?'))))
///
/// Or like this if it is any other ExpressibleByNilLiteral type:
///
Expand All @@ -1296,13 +1299,10 @@ static void diagSyntacticUseRestrictions(const Expr *E, const DeclContext *DC,
bool isTypeCheckedOptionalNil(Expr *E) {
if (dyn_cast<NilLiteralExpr>(E)) return true;

auto CE = dyn_cast<ApplyExpr>(E->getSemanticsProvidingExpr());
if (!CE || !CE->isImplicit())
return false;

// First case -- Optional.none
if (auto DRE = dyn_cast<DeclRefExpr>(CE->getSemanticFn()))
return DRE->getDecl() == Ctx.getOptionalNoneDecl();
if (auto *DSCE = dyn_cast_or_null<DotSyntaxCallExpr>(E->getSemanticsProvidingExpr())) {
if (auto *DRE = dyn_cast<DeclRefExpr>(DSCE->getSemanticFn()))
return DRE->getDecl() == Ctx.getOptionalNoneDecl();
}

return false;
}
Expand Down Expand Up @@ -1350,9 +1350,10 @@ static void diagSyntacticUseRestrictions(const Expr *E, const DeclContext *DC,
(isTypeCheckedOptionalNil(lhs) &&
(subExpr = isImplicitPromotionToOptional(rhs)))) {
bool isTrue = calleeName == "!=" || calleeName == "!==";
bool isNilLiteral = isa<NilLiteralExpr>(lhs) || isa<NilLiteralExpr>(rhs);

Ctx.Diags.diagnose(DRE->getLoc(), diag::nonoptional_compare_to_nil,
subExpr->getType(), isTrue)
subExpr->getType(), isNilLiteral, isTrue)
.highlight(lhs->getSourceRange())
.highlight(rhs->getSourceRange());
return;
Expand Down
9 changes: 9 additions & 0 deletions test/Constraints/diagnostics.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -746,9 +746,13 @@ class C_44203 {
_ = (i === nil) // expected-error {{value of type 'Int?' cannot be compared by reference; did you mean to compare by value?}} {{12-15===}}
_ = (bytes === nil) // expected-error {{type 'UnsafeMutablePointer<Int>' is not optional, value can never be nil}}
_ = (self === nil) // expected-warning {{comparing non-optional value of type 'AnyObject' to 'nil' always returns false}}
_ = (self === .none) // expected-warning {{comparing non-optional value of type 'AnyObject' to 'Optional.none' always returns false}}
_ = (self === Optional.none) // expected-warning {{comparing non-optional value of type 'AnyObject' to 'Optional.none' always returns false}}
_ = (i !== nil) // expected-error {{value of type 'Int?' cannot be compared by reference; did you mean to compare by value?}} {{12-15=!=}}
_ = (bytes !== nil) // expected-error {{type 'UnsafeMutablePointer<Int>' is not optional, value can never be nil}}
_ = (self !== nil) // expected-warning {{comparing non-optional value of type 'AnyObject' to 'nil' always returns true}}
_ = (self !== .none) // expected-warning {{comparing non-optional value of type 'AnyObject' to 'Optional.none' always returns true}}
_ = (self !== Optional.none) // expected-warning {{comparing non-optional value of type 'AnyObject' to 'Optional.none' always returns true}}
}
}

Expand All @@ -757,6 +761,11 @@ func nilComparison(i: Int, o: AnyObject) {
_ = nil == i // expected-warning {{comparing non-optional value of type 'Int' to 'nil' always returns false}}
_ = i != nil // expected-warning {{comparing non-optional value of type 'Int' to 'nil' always returns true}}
_ = nil != i // expected-warning {{comparing non-optional value of type 'Int' to 'nil' always returns true}}

_ = i == Optional.none // expected-warning {{comparing non-optional value of type 'Int' to 'Optional.none' always returns false}}
_ = Optional.none == i // expected-warning {{comparing non-optional value of type 'Int' to 'Optional.none' always returns false}}
_ = i != Optional.none // expected-warning {{comparing non-optional value of type 'Int' to 'Optional.none' always returns true}}
_ = Optional.none != i // expected-warning {{comparing non-optional value of type 'Int' to 'Optional.none' always returns true}}

// FIXME(integers): uncomment these tests once the < is no longer ambiguous
// _ = i < nil // _xpected-error {{type 'Int' is not optional, value can never be nil}}
Expand Down
15 changes: 15 additions & 0 deletions test/expr/cast/nil_value_to_optional.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ func markUsed<T>(_ t: T) {}

markUsed(t != nil) // expected-warning {{comparing non-optional value of type 'Bool' to 'nil' always returns true}}
markUsed(f != nil) // expected-warning {{comparing non-optional value of type 'Bool' to 'nil' always returns true}}
markUsed(t != Optional.none) // expected-warning {{comparing non-optional value of type 'Bool' to 'Optional.none' always returns true}}
markUsed(f != Optional.none) // expected-warning {{comparing non-optional value of type 'Bool' to 'Optional.none' always returns true}}

class C : Equatable {}

Expand All @@ -16,6 +18,9 @@ func == (lhs: C, rhs: C) -> Bool {

func test(_ c: C) {
if c == nil {} // expected-warning {{comparing non-optional value of type 'C' to 'nil' always returns false}}
if c == .none {} // expected-warning {{comparing non-optional value of type 'C' to 'Optional.none' always returns false}}
if c == Optional.none {} // expected-warning {{comparing non-optional value of type 'C' to 'Optional.none' always returns false}}
if c == C?.none {} // expected-warning {{comparing non-optional value of type 'C' to 'Optional.none' always returns false}}
}

class D {}
Expand All @@ -30,3 +35,13 @@ _ = dopt == nil
_ = diuopt == nil
_ = diuopt is ExpressibleByNilLiteral // expected-warning {{'is' test is always true}}
_ = produceD() is ExpressibleByNilLiteral // expected-warning {{'is' test is always true}}

enum E {
case none
}
func test(_ e: E) {
_ = e == .none
_ = e == E.none
_ = e == Optional.none // expected-warning {{comparing non-optional value of type 'E' to 'Optional.none' always returns false}}
_ = e == E?.none // expected-warning {{comparing non-optional value of type 'E' to 'Optional.none' always returns false}}
}
5 changes: 5 additions & 0 deletions test/expr/expressions.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -767,6 +767,11 @@ _ = nil == Int.self // expected-warning {{comparing non-optional value of type
_ = Int.self != nil // expected-warning {{comparing non-optional value of type 'any Any.Type' to 'nil' always returns true}}
_ = nil != Int.self // expected-warning {{comparing non-optional value of type 'any Any.Type' to 'nil' always returns true}}

_ = Int.self == .none // expected-warning {{comparing non-optional value of type 'any Any.Type' to 'Optional.none' always returns false}}
_ = .none == Int.self // expected-warning {{comparing non-optional value of type 'any Any.Type' to 'Optional.none' always returns false}}
_ = Int.self != .none // expected-warning {{comparing non-optional value of type 'any Any.Type' to 'Optional.none' always returns true}}
_ = .none != Int.self // expected-warning {{comparing non-optional value of type 'any Any.Type' to 'Optional.none' always returns true}}

// <rdar://problem/19032294> Disallow postfix ? when not chaining
func testOptionalChaining(_ a : Int?, b : Int!, c : Int??) {
_ = a? // expected-error {{optional chain has no effect, expression already produces 'Int?'}} {{8-9=}}
Expand Down