Attribute @_implements & deriving enum equality not-named == #8735

graydon · 2017-04-13T00:53:32Z

This implements (non-user-facing) attribute @_implements(Protocol, DeclName) that can be used to conform to a protocol requirement using a decl with a different name than the requirement.

This might sound weird, but it has (at least?) two uses we've already hit:

Conforming to two protocols that use the same name for different operations (this arises when comparing floating point values)
Conforming to a protocol that has a requirement that's an operator-name, without adding a new operator to the global operator list (this happens via implicit Equatable conformances on enums, and causes a lot of needless top-level inter-file dependencies)

After initial post and review, I've ticked off the following todos:

Limit lookup to only happen when locating witnesses
Limit lookup to the Context (protocol) name specified; currently it ignores that part of the name
Get it working for Operator names (including protocol members)
Add ~~~lots~~~ some more tests and fix all bugs those reveal
Check that @_implements only mentions protocols the decl conforms to

rdar://31607695

This was originally motivated by 30959593 (operators defeat incrementality), and I've now tacked on to the end a change that @_implements the derived enum == operator in terms of a different function (creatively named __derived_enum_equals)

slavapestov · 2017-04-13T00:55:49Z

lib/AST/NameLookup.cpp

@@ -1067,6 +1068,13 @@ void MemberLookupTable::addMember(Decl *member) {
  // Add this declaration to the lookup set under its compound name and simple
  // name.
  vd->getFullName().addToLookupTable(Lookup, vd);
+
+  // Add this declaration to the lookup set under any @_implements name as well


How do you prevent name lookup on concrete types from finding the decl? We only want to see the implements name while doing conformance checks no?

Correct! Forgot to put this on the todo list. Thanks!

I think the right place to hook this up is not inside name lookup, but in lib/Sema/TypeCheckProtocol.cpp, in particular lookupValueWitnesses(). When you begin conformance checking, maybe walk the members to collect all _implements attributes, building a mapping from requirement to witness inside the conformance. Then lookupValueWitness() can avoid most of the work by checking the pre-populated map first.

Good suggestion! Fixed.

slavapestov · 2017-04-13T04:50:51Z

lib/Parse/ParseDecl.cpp

+  // We parse a ParsedDeclName here just to make sure it's well formed early; we
+  // store it in the attr as a StringRef and re-parse on use, so as not to leak
+  // the ParsedDeclName type into Attr.h
+  ParsedDeclName parsedName = parseDeclName(ImplName.getValue());


Perhaps the semantic checking can be done in lib/Sema/TypeCheckAttr.cpp, so you're not simply throwing away the result.

slavapestov · 2017-04-13T04:51:35Z

I would also suggest adding some semantic checks to Sema that _implements only mentions requirements of protocols that the type conforms to.

Also do we want to allow _implements inside extensions? This might open up some interesting corner cases.

graydon · 2017-04-14T03:23:29Z

Pushed an update that reworks parsing and definition to handle operator names, permits the bare-minimum feature of @implements(Equatable, ==(:_:)). More comments welcome!

graydon · 2017-04-14T20:29:57Z

Pushed update to limit the name visibility to witness lookup.

graydon · 2017-04-16T03:00:10Z

Pushed variant that does everything on the todo list, with some new tests. Going to look at synthesizing this for the motivating case now (dependency-edge cutting) but please let me know if there's anything else you want to see for an initial merge.

graydon · 2017-04-17T01:21:36Z

Pushed change that derives func== on enums using @_implements, cutting the provides-top-level dependency edge.

slavapestov · 2017-04-17T22:30:28Z

lib/Sema/DerivedConformanceEquatableHashable.cpp

@@ -196,7 +196,8 @@ deriveEquatable_enum_eq(TypeChecker &tc, Decl *parentDecl, EnumDecl *enumDecl) {
  //   case A, B, C
  //
  //   @derived
-  //   func ==(a: SomeEnum<T...>, b: SomeEnum<T...>) -> Bool {
+  //   @_implements(Equatable, ==(_:_:))
+  //   func _DerivedEnumEquals(a: SomeEnum<T...>, b: SomeEnum<T...>) -> Bool {


Is this going to look ugly in generated interface printing? Maybe we could special case that to print this as == (or just skip synthesized decls altogether).

I think it's already marked implicit, so most people won't see it. There are just a few tests that try to print implicit declarations as well.

Yeah, it's implicit, no printing by default.

Drive-by thought:

Wouldn't it be neat to have this be consistent with the "__Anonymous_field" and "__Unnamed_union" terminology introduced in #6935? (I guess it'd be something like "__Derived_conformance_enum_equals".)

…on that note, it's not a type, so lowercase would make more sense.

Changed to __derived_enum_equals after checking with Jordan.

slavapestov · 2017-04-17T22:30:45Z

lib/Sema/TypeCheckAttr.cpp

+  ProtoTypeLoc.setType(T);
+
+  // Definite error-types were already diagnosed in resolveType.
+  if (!T || T->is<ErrorType>())


or hasErrorType(), even

(Assuming you mean hasError(), and to replace the second disjunct with that test, then: fixed)

slavapestov · 2017-04-17T22:31:41Z

lib/Sema/TypeCheckProtocol.cpp

+
+  // Do an initial check to see if there are any @_implements remappings
+  // for this requirement.
+  if (req->isProtocolRequirement() && req->hasName()) {


Maybe factor this out into a new lookupValueWitnessViaImplementsAttr() and use early return to avoid the "} cascade of death"

slavapestov · 2017-04-17T22:31:51Z

lib/Sema/TypeCheckRequest.cpp

@@ -16,6 +16,7 @@
 //===----------------------------------------------------------------------===//
 #include "swift/Sema/TypeCheckRequest.h"
 #include "swift/AST/Decl.h"
+#include "swift/AST/TypeRepr.h"


Is this necessary?

Yep. the call to std::get<0>(getTypeResolutionPayload())->getLoc() needs it.

slavapestov · 2017-04-17T22:32:13Z

This looks great, nice job!

graydon · 2017-04-17T23:38:44Z

Pushed variant addressing recent review comments from Slava.

graydon · 2017-04-17T23:39:45Z

@swift-ci please test

swift-ci · 2017-04-17T23:40:07Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - b2cb2313e90495a174831abdc2b0567c6023f2dd
Test requested by - @graydon

swift-ci · 2017-04-17T23:52:54Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - b2cb2313e90495a174831abdc2b0567c6023f2dd
Test requested by - @graydon

…ames.

graydon · 2017-04-18T18:51:44Z

@swift-ci please smoke test

DougGregor · 2017-04-17T04:02:25Z

include/swift/AST/Attr.def

@@ -267,6 +267,11 @@ SIMPLE_DECL_ATTR(discardableResult, DiscardableResult,

 SIMPLE_DECL_ATTR(GKInspectable, GKInspectable, OnVar, 66)

+DECL_ATTR(_implements, Implements,
+          OnFunc | OnVar | OnSubscript | OnTypeAlias
+          | NotSerialized | UserInaccessible,


If the intent is for these declarations to be invisible to normal name lookup (is that the intent?) or hide it from code completion, then you'll probably need to serialize the attribute.

The attribute isn't what triggers either of those things, so we're okay.

DougGregor · 2017-04-17T04:18:14Z

include/swift/AST/KnownIdentifiers.def

@@ -90,6 +90,7 @@ IDENTIFIER_WITH_NAME(NativeClassLayout, "_NativeClass")
 // Operators
 IDENTIFIER_WITH_NAME(MatchOperator, "~=")
 IDENTIFIER_WITH_NAME(EqualsOperator, "==")
+IDENTIFIER_WITH_NAME(DerivedEnumEquals, "_DerivedEnumEquals")


Lowercase _derivedEnumEquals because it's a function (not a type)?

renamed to __derived_enum_equals per conversation w/ Jordan

xwu · 2017-04-18T23:45:31Z

lib/Sema/DerivedConformanceEquatableHashable.cpp

@@ -196,7 +196,9 @@ deriveEquatable_enum_eq(TypeChecker &tc, Decl *parentDecl, EnumDecl *enumDecl) {
  //   case A, B, C
  //
  //   @derived
-  //   func ==(a: SomeEnum<T...>, b: SomeEnum<T...>) -> Bool {
+  //   @_implements(Equatable, ==(_:_:))
+  //   func __derived_conformance_enum_equals(a: SomeEnum<T...>,


s/__derived_conformance/__derived/

…ts).

graydon · 2017-04-19T07:00:15Z

Pushed variant with fixed name and change to user-availability bit such that code completion never tells the user about __derived_enum_equals.

graydon · 2017-04-19T07:01:08Z

@swift-ci please test

swift-ci · 2017-04-19T07:01:26Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 45b5fb11f9c2c72f88283cceb3aafabc56915b5f
Test requested by - @graydon

swift-ci · 2017-04-19T07:03:15Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 45b5fb11f9c2c72f88283cceb3aafabc56915b5f
Test requested by - @graydon

graydon · 2017-04-19T15:37:01Z

@swift-ci please test

graydon · 2017-04-19T18:18:49Z

r=dgregor, merging.

graydon requested review from DougGregor and jrose-apple April 13, 2017 00:53

slavapestov reviewed Apr 13, 2017

View reviewed changes

graydon force-pushed the rdar-30959593-operators-defeat-incrementality branch from b2cb231 to cd53e8b Compare April 14, 2017 03:19

graydon force-pushed the rdar-30959593-operators-defeat-incrementality branch from cd53e8b to 51561f0 Compare April 14, 2017 20:01

graydon force-pushed the rdar-30959593-operators-defeat-incrementality branch from 51561f0 to f06277d Compare April 16, 2017 02:53

graydon changed the title ~~WIP / Do Not Merge Yet: Attribute @_implements~~ Attribute @_implements Apr 16, 2017

graydon force-pushed the rdar-30959593-operators-defeat-incrementality branch from f06277d to bcd1913 Compare April 17, 2017 01:18

graydon changed the title ~~Attribute @_implements~~ Attribute @_implements & deriving enum equality not-named == Apr 17, 2017

slavapestov reviewed Apr 17, 2017

View reviewed changes

graydon force-pushed the rdar-30959593-operators-defeat-incrementality branch from bcd1913 to 45b5fb1 Compare April 17, 2017 23:38

graydon added 5 commits April 18, 2017 11:12

Forward declare TypeRepr in TypeLoc.h, adjust other code to compensate.

3a79d83

Add @_implements decl attribute.

c71295a

Wire up @_implements to witness lookup.

6eddd7f

Add mode to Parser::parseUnqualifiedDeclName to handle operator BaseN…

e191b92

…ames.

Add mode to Parser::parseUnqualifiedDeclName for 0-arg compound names.

4d9ea18

graydon added 6 commits April 18, 2017 11:12

Parse @_implements(Proto, DeclName)

3d5c995

Typecheck contents of @_implements attribute.

db51588

Add testcase for @_implements

1b3815e

Add testcase for @_implements parser diagnostics.

0894464

Add testcase for @_implements typechecking diagnostics.

7af4539

Print @_implements to AST printer.

b662c3a

graydon force-pushed the rdar-30959593-operators-defeat-incrementality branch from 45b5fb1 to d65027f Compare April 18, 2017 18:13

DougGregor reviewed Apr 18, 2017

View reviewed changes

xwu reviewed Apr 18, 2017

View reviewed changes

graydon added 3 commits April 18, 2017 23:21

Move IsUserAccessible bit from VarDecl up to ValueDecl (and adjust bi…

c25210f

…ts).

Don't code-complete any ValueDecl that's not user accessible.

3955f5e

Derive @_implements(Equatable, ==(_:_:)) _DerivedEnumEquals, not func==.

e6027ee

graydon force-pushed the rdar-30959593-operators-defeat-incrementality branch from d65027f to e6027ee Compare April 19, 2017 06:54

graydon requested review from slavapestov and DougGregor April 19, 2017 15:46

graydon merged commit 6a94c9f into swiftlang:master Apr 19, 2017

graydon deleted the rdar-30959593-operators-defeat-incrementality branch September 13, 2017 16:42

ahoppen mentioned this pull request Dec 27, 2017

[Sema] Translate names in unused result warnings of implicit functions #13622

Merged

jrose-apple mentioned this pull request Aug 4, 2018

Don't use __derived_enum_equals for a resilient synthesized '==' #18508

Merged

xwu mentioned this pull request Apr 25, 2022

[SR-6981] @_implements doesn't work in extensions #49529

Closed

Attribute @_implements & deriving enum equality not-named == #8735

Attribute @_implements & deriving enum equality not-named == #8735

Conversation

graydon commented Apr 13, 2017 • edited by ahoppen Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slavapestov commented Apr 13, 2017

graydon commented Apr 14, 2017

graydon commented Apr 14, 2017

graydon commented Apr 16, 2017

graydon commented Apr 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slavapestov commented Apr 17, 2017

graydon commented Apr 17, 2017

graydon commented Apr 17, 2017

swift-ci commented Apr 17, 2017

swift-ci commented Apr 17, 2017

graydon commented Apr 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graydon commented Apr 19, 2017

graydon commented Apr 19, 2017

swift-ci commented Apr 19, 2017

swift-ci commented Apr 19, 2017

graydon commented Apr 19, 2017

graydon commented Apr 19, 2017

graydon commented Apr 13, 2017 •

edited by ahoppen

Loading