[Sema] Add warning for ambiguous value assignment when using Optional (#21621)

* [sema] emit a diag if the enum case matches Optional<T>.none

* [test] update tests

* [sema] fix indent

* [test] fix indent

* [test] add more test cases

* [test] add even more test cases

* [sema] move the check to CSApply

* [diag] update diagnostic message

* [test] update tests

* [test] fix conflicts

* [diag] reflow lines

* [sema] reindent using spaces

* [test] adds new line

* [diag] update diagnostic message

* [sema] add support for structs as well

* [test] add more test cases

* [sema] check for enum assoc values

* [test] add more test cases

* [diag] add fixit notes

* [sema] emit fix its

* [diag] rename diag names

* [sema] fit within 80 char line limit

* [sema] use baseUnwrappedType's name directly

* [test] adds nested generic enum tests

* [test] fix indent

* [test] adds fixit check

* [test] re-indent some enums

* [sema] [csapply] extract code into a separate function

* [sema] [csapply] remove redundant vardecl check

* [sema] [csapply] reindent

* [sema] [csapply] removes extra line

* [sema] [csapply] use cantype & check for extension on Optional

* [diag] update diagnostic message

* [sema] [csapply] fix ident

* [test] update tests

* [sema] [csapply] fix typo and remove redundant isOptional check

* [sema] [csapply] update var name & comments

* [sema] [csapply] bring back isOptional check

* [test] add expected-note for fix-its

* [sema] [csapply] fix a crash

* [sema] [csapply] move isOptional check outside

* [test] fix indent

* [test] fix typo

* [sema] [csapply] use baseTyUnwrapped for fixit

* [test] fix columns for fixits

* [test] update column numbers

* [sema] [csapply] move code out of for loop

Author: theblixguy

include/swift/AST/DiagnosticsSema.def

@@ -1391,6 +1391,14 @@ ERROR(pattern_binds_no_variables,none,
       "variables",
       (unsigned))
 
+WARNING(optional_ambiguous_case_ref,none,
+        "assuming you mean '%0.%2'; did you mean '%1.%2' instead?",
+        (StringRef, StringRef, StringRef))
+NOTE(optional_fixit_ambiguous_case_ref,none,
+     "explicitly specify 'Optional' to silence this warning", ())
+NOTE(type_fixit_optional_ambiguous_case_ref,none,
+     "use '%0.%1' instead", (StringRef, StringRef))
+
 ERROR(nscoding_unstable_mangled_name,none,
       "%select{private|fileprivate|nested|local}0 class %1 has an "
       "unstable name when archiving via 'NSCoding'",

lib/Sema/CSApply.cpp

@@ -2514,10 +2514,94 @@ namespace {
             /*implicit=*/expr->isImplicit(), Type(), getType);
         result = finishApply(apply, Type(), cs.getConstraintLocator(expr));
       }
+			
+      // Check for ambiguous member if the base is an Optional
+      if (baseTy->getOptionalObjectType()) {
+        diagnoseAmbiguousNominalMember(baseTy, result);
+      }
 
       return coerceToType(result, resultTy, cs.getConstraintLocator(expr));
     }
 
+    /// Diagnose if the base type is optional, we're referring to a nominal
+    /// type member via the dot syntax and the member name matches
+    /// Optional<T>.{member name}
+    void diagnoseAmbiguousNominalMember(Type baseTy, Expr *result) {
+      if (auto baseTyUnwrapped = baseTy->lookThroughAllOptionalTypes()) {
+        // Return if the base type doesn't have a nominal type decl
+        if (!baseTyUnwrapped->getNominalOrBoundGenericNominal()) {
+          return;
+        }
+        
+        // Otherwise, continue digging
+        if (auto DSCE = dyn_cast<DotSyntaxCallExpr>(result)) {
+          auto calledValue = DSCE->getCalledValue();
+          auto isOptional = false;
+          Identifier memberName;
+          
+          // Try cast the assigned value to an enum case
+          //
+          // This will always succeed if the base is Optional<T> & the
+          // assigned case comes from Optional<T>
+          if (auto EED = dyn_cast<EnumElementDecl>(calledValue)) {
+            isOptional = EED->getParentEnum()->isOptionalDecl();
+            memberName = EED->getBaseName().getIdentifier();
+          }
+          
+          // Return if the enum case doesn't come from Optional<T>
+          if (!isOptional) {
+            return;
+          }
+          
+          // Look up the enum case in the unwrapped type to check if it exists
+          // as a member
+          auto baseTyNominalDecl = baseTyUnwrapped
+                                   ->getNominalOrBoundGenericNominal();
+          auto &tc = cs.getTypeChecker();
+          auto results = tc.lookupMember(baseTyNominalDecl->getModuleContext(),
+                                         baseTyUnwrapped,
+                                         memberName,
+                                         defaultMemberLookupOptions);
+          
+          // Lookup didn't find anything, so return
+          if (results.empty()) {
+            return;
+          }
+          
+          if (auto member = results.front().getValueDecl()) {
+            // Lookup returned a member that is an instance member,
+            // so return
+            if (member->isInstanceMember()) {
+              return;
+            }
+            
+            // Return if the member is an enum case w/ assoc values, as we only
+            // care (for now) about cases with no assoc values (like none)
+            if (auto EED = dyn_cast<EnumElementDecl>(member)) {
+              if (EED->hasAssociatedValues()) {
+                return;
+              }
+            }
+            
+            // Emit a diagnostic with some fixits
+            auto baseTyName = baseTy->getCanonicalType().getString();
+            auto baseTyUnwrappedName = baseTyUnwrapped->getString();
+            auto loc = DSCE->getLoc();
+            auto startLoc = DSCE->getStartLoc();
+            
+            tc.diagnose(loc, swift::diag::optional_ambiguous_case_ref,
+                        baseTyName, baseTyUnwrappedName, memberName.str());
+            
+            tc.diagnose(loc, swift::diag::optional_fixit_ambiguous_case_ref)
+              .fixItInsert(startLoc, "Optional");
+            tc.diagnose(loc, swift::diag::type_fixit_optional_ambiguous_case_ref,
+                        baseTyUnwrappedName, memberName.str())
+              .fixItInsert(startLoc, baseTyUnwrappedName);
+          }
+        }
+      }
+    }
+    
   private:
     /// A list of "suspicious" optional injections that come from
     /// forced downcasts.

test/decl/enum/enumtest.swift

@@ -333,3 +333,106 @@ enum Lens<T> {
 enum HasVariadic {
   case variadic(x: Int...) // expected-error {{variadic enum cases are not supported}}
 }
+
+// SR-2176
+enum Foo {
+  case bar
+  case none
+}
+
+let _: Foo? = .none // expected-warning {{assuming you mean 'Optional<Foo>.none'; did you mean 'Foo.none' instead?}}
+// expected-note@-1 {{explicitly specify 'Optional' to silence this warning}} {{15-15=Optional}}
+// expected-note@-2 {{use 'Foo.none' instead}} {{15-15=Foo}}
+let _: Foo?? = .none // expected-warning {{assuming you mean 'Optional<Optional<Foo>>.none'; did you mean 'Foo.none' instead?}}
+// expected-note@-1 {{explicitly specify 'Optional' to silence this warning}} {{16-16=Optional}}
+// expected-note@-2 {{use 'Foo.none' instead}} {{16-16=Foo}}
+
+let _: Foo = .none // ok
+let _: Foo = .bar // ok
+let _: Foo? = .bar // ok
+let _: Foo?? = .bar // ok
+let _: Foo = Foo.bar // ok
+let _: Foo = Foo.none // ok
+let _: Foo? = Foo.none // ok
+let _: Foo?? = Foo.none // ok
+
+func baz(_: Foo?) {}
+baz(.none) // expected-warning {{assuming you mean 'Optional<Foo>.none'; did you mean 'Foo.none' instead?}}
+// expected-note@-1 {{explicitly specify 'Optional' to silence this warning}} {{5-5=Optional}}
+// expected-note@-2 {{use 'Foo.none' instead}} {{5-5=Foo}}
+
+let test: Foo? = .none // expected-warning {{assuming you mean 'Optional<Foo>.none'; did you mean 'Foo.none' instead?}}
+// expected-note@-1 {{explicitly specify 'Optional' to silence this warning}} {{18-18=Optional}}
+// expected-note@-2 {{use 'Foo.none' instead}} {{18-18=Foo}}
+let answer = test == .none // expected-warning {{assuming you mean 'Optional<Foo>.none'; did you mean 'Foo.none' instead?}}
+// expected-note@-1 {{explicitly specify 'Optional' to silence this warning}} {{22-22=Optional}}
+// expected-note@-2 {{use 'Foo.none' instead}} {{22-22=Foo}}
+
+enum Bar {
+  case baz
+}
+
+let _: Bar? = .none // ok
+let _: Bar?? = .none // ok
+let _: Bar? = .baz // ok
+let _: Bar?? = .baz // ok
+let _: Bar = .baz // ok
+
+enum AnotherFoo {
+  case none(Any)
+}
+
+let _: AnotherFoo? = .none // ok
+let _: AnotherFoo? = .none(0) // ok
+
+struct FooStruct {
+  static let none = FooStruct()
+  static let one = FooStruct()
+}
+
+let _: FooStruct? = .none // expected-warning {{assuming you mean 'Optional<FooStruct>.none'; did you mean 'FooStruct.none' instead?}}
+// expected-note@-1 {{explicitly specify 'Optional' to silence this warning}} {{21-21=Optional}}
+// expected-note@-2 {{use 'FooStruct.none' instead}} {{21-21=FooStruct}}
+let _: FooStruct?? = .none // expected-warning {{assuming you mean 'Optional<Optional<FooStruct>>.none'; did you mean 'FooStruct.none' instead?}}
+// expected-note@-1 {{explicitly specify 'Optional' to silence this warning}} {{22-22=Optional}}
+// expected-note@-2 {{use 'FooStruct.none' instead}} {{22-22=FooStruct}}
+let _: FooStruct = .none // ok
+let _: FooStruct = .one // ok
+let _: FooStruct? = .one // ok
+let _: FooStruct?? = .one // ok
+
+struct NestedBazEnum {
+  enum Baz {
+    case one
+    case none
+  }
+}
+
+let _: NestedBazEnum.Baz? = .none // expected-warning {{assuming you mean 'Optional<NestedBazEnum.Baz>.none'; did you mean 'NestedBazEnum.Baz.none' instead?}}
+// expected-note@-1 {{explicitly specify 'Optional' to silence this warning}} {{29-29=Optional}}
+// expected-note@-2 {{use 'NestedBazEnum.Baz.none' instead}} {{29-29=NestedBazEnum.Baz}}
+let _: NestedBazEnum.Baz?? = .none // expected-warning {{assuming you mean 'Optional<Optional<NestedBazEnum.Baz>>.none'; did you mean 'NestedBazEnum.Baz.none' instead?}}
+// expected-note@-1 {{explicitly specify 'Optional' to silence this warning}} {{30-30=Optional}}
+// expected-note@-2 {{use 'NestedBazEnum.Baz.none' instead}} {{30-30=NestedBazEnum.Baz}}
+let _: NestedBazEnum.Baz = .none // ok
+let _: NestedBazEnum.Baz = .one // ok
+let _: NestedBazEnum.Baz? = .one // ok
+let _: NestedBazEnum.Baz?? = .one // ok
+
+struct NestedBazEnumGeneric {
+  enum Baz<T> {
+    case one
+    case none
+  }
+}
+
+let _: NestedBazEnumGeneric.Baz<Int>? = .none // expected-warning {{assuming you mean 'Optional<NestedBazEnumGeneric.Baz<Int>>.none'; did you mean 'NestedBazEnumGeneric.Baz<Int>.none' instead?}}
+// expected-note@-1 {{explicitly specify 'Optional' to silence this warning}} {{41-41=Optional}}
+// expected-note@-2 {{use 'NestedBazEnumGeneric.Baz<Int>.none' instead}} {{41-41=NestedBazEnumGeneric.Baz<Int>}}
+let _: NestedBazEnumGeneric.Baz<Int>?? = .none // expected-warning {{assuming you mean 'Optional<Optional<NestedBazEnumGeneric.Baz<Int>>>.none'; did you mean 'NestedBazEnumGeneric.Baz<Int>.none' instead?}}
+// expected-note@-1 {{explicitly specify 'Optional' to silence this warning}} {{42-42=Optional}}
+// expected-note@-2 {{use 'NestedBazEnumGeneric.Baz<Int>.none' instead}} {{42-42=NestedBazEnumGeneric.Baz<Int>}}
+let _: NestedBazEnumGeneric.Baz<Int> = .none // ok
+let _: NestedBazEnumGeneric.Baz<Int> = .one // ok
+let _: NestedBazEnumGeneric.Baz<Int>? = .one // ok
+let _: NestedBazEnumGeneric.Baz<Int>?? = .one // ok