Inherit doc comments from default implementations of inherited protocols #73066

ahoppen · 2024-04-16T21:16:14Z

For example when we have

protocol BaseProtocol {}

extension BaseProtocol {
  /// Say hello
  func hello() { }
}
struct MyStruct: BaseProtocol {
  func hello() {}
}

Then MyStruct.hello should inherit the doc comment from the implementation of BaseProtocol.hello. Currently no doc comment is associated with MyStruct.hello.

rdar://126240546

The changes in offsets in the test cases is because init(rawValue:) now inherits the doc comment of RawRepresentable, which shifts the lines in generated interfaces around.

bnbarham · 2024-04-16T22:05:57Z

lib/AST/DocComment.cpp

@@ -426,24 +426,24 @@ class CommentProviderFinder final {
    return std::nullopt;
  }

+  /// Check if there is an inherited protocol that has a default implementaiton


Suggested change

/// Check if there is an inherited protocol that has a default implementaiton

/// Check if there is an inherited protocol that has a default implementation

bnbarham · 2024-04-16T23:27:34Z

test/SourceKit/CursorInfo/cursor_inherit_doc_from_protocol_default_impl.swift

@@ -0,0 +1,34 @@
+protocol BaseProtocol {}


Could add a test for the case where there is both a requirement and a default

ahoppen · 2024-04-17T20:59:16Z

@swift-ci Please smoke test

hamishknight · 2024-04-17T21:16:46Z

lib/AST/DocComment.cpp

+    NominalTypeDecl *nominalType =
+        dyn_cast_or_null<NominalTypeDecl>(VD->getDeclContext()->getAsDecl());


What if VD is defined in an extension? Do we want to support that?

We do with the lines below, right?

https://github.com/apple/swift/pull/73066/files#diff-ff85ad3908ab33036e814561ae7b5cec9f0be34b4de5106b989957b2d5ee5afeR434-R436

That only handles protocol extensions, right? I was thinking of e.g a struct extension

lib/AST/DocComment.cpp

ahoppen · 2024-04-18T03:46:19Z

test/SymbolGraph/Relationships/Synthesized/RemoteInheritedDocs.swift

-// `RemoteP.P` also has an extension with a default implementation for `extraFunc` that does have
-// docs, but overriding it here should prevent those from appearing
-


@QuietMisdreavus This comments seems to imply the exact opposite of what I want to achieve in this PR. Is there any reason why we didn’t want to inherit documentation from extensions?

For context, the concrete example I want to fix with this PR is that this implementation of _ArrayProtocol.filter https://github.com/apple/swift/blob/c531f2914fda0b84d8af091a0081799101f76d9b/stdlib/public/core/ArrayType.swift#L73-L78 should inherit the documentation from the documentation of Sequence.filter, which is implemented in an extension https://github.com/apple/swift/blob/5df0f053d95c1143ac34ac88faafdfa3902a3224/stdlib/public/core/Sequence.swift#L717-L739

cc @franklinsch (and possibly also @bitjammer for the underlying discussion of inheriting docs in the first place)

I think it should respect the presence of the -skip-inherited-docs flag, but otherwise i don't remember off the top of my head.

I think we already respect the -skip-inherited-docs flag because we’re not calling into getDocCommentProvidingDecl if it is set and this PR only modifies getDocCommentProvidingDecl

https://github.com/apple/swift/blob/9fdb0e79e2dab72fdc01f8fa50d9cc249a48a5a0/lib/SymbolGraphGen/Symbol.cpp#L359-L365

hamishknight · 2024-04-18T10:59:59Z

lib/AST/DocComment.cpp

+      if (!memberComparisonTy->matches(vdComparisonTy, TypeMatchFlags::AllowOverride)) {
        continue;


Should this be the other way around? e.g allowing:

protocol P {} extension P { /// hello func foo() -> Int? { 0 } } struct S: P { func foo() -> Int { 0 } }

But not:

protocol P {} extension P { /// hello func foo() -> Int { 0 } } struct S: P { func foo() -> Int? { 0 } }

Not that these are really overrides anyway, but if we want similar type-matching rules, then I think it makes sense to flip the ordering.

Oh, yes. I thought that I had this case covered with the throws case but apparently throws works either way 🤷🏽

hamishknight · 2024-04-18T11:09:29Z

lib/AST/DocComment.cpp

+    nominalType->lookupQualified(nominalType, DeclNameRef(VD->getName()),
+                                 VD->getLoc(), NLOptions::NL_ProtocolMembers,
+                                 members);


Thought of a couple more edge cases :)

static mismatch

protocol P {} extension P { /// hello static func foo() {} } struct S: P { func foo() {} } func bar(_ x: S) { x.foo() }

enum case matching a function (if we want this to work it would need to be a static function)

protocol P {} extension P { /// hello func foo(_ x: Int) -> E { .foo(0) } } enum E: P { case foo(Int) } func bar(_ x: E) { E.foo(0) }

this is quite silly, but technically this would match:

class C { /// hello struct foo {} } class D: C { var foo: C.foo.Type { C.foo.self } } func bar(_ x: D) { let y = x.foo }

Good cases, thanks. Case 3 does inherit but I decided to not care and not add a test case for it. If it broke, I don’t think we would care.

hamishknight · 2024-04-18T11:22:56Z

lib/AST/DocComment.cpp

+    if (auto fnTy = vdComparisonTy->getAs<AnyFunctionType>()) {
+      // Strip off the 'Self' parameter.
+      vdComparisonTy = fnTy->getResult();
+    }


Wonder if we ought to have a utility for removing the curried self, could be used in a couple of other places:

https://github.com/apple/swift/blob/15fd443be60cfb52852c853c4fa0d6a1622ada10/lib/Sema/TypeCheckProtocol.cpp#L4894-L4896

https://github.com/apple/swift/blob/15fd443be60cfb52852c853c4fa0d6a1622ada10/lib/Sema/AssociatedTypeInference.cpp#L1990-L1993

https://github.com/apple/swift/blob/15fd443be60cfb52852c853c4fa0d6a1622ada10/lib/AST/Decl.cpp#L4937-L4939

ahoppen · 2024-04-18T22:48:59Z

@swift-ci Please smoke test

ahoppen · 2024-04-19T05:20:03Z

@swift-ci Please smoke test

slavapestov · 2024-04-19T21:33:34Z

lib/AST/DocComment.cpp

@@ -426,24 +426,49 @@ class CommentProviderFinder final {
    return std::nullopt;
  }

+  /// Check if there is an inherited protocol that has a default implementation
+  /// of `VD` with a doc comment.
  std::optional<ResultWithDecl> findDefaultProvidedDecl(const ValueDecl *VD) {


This is just duplicating logic in the witness checker. Default witnesses are recorded in the ProtocolDecl.

See TypeChecker::inferDefaultWitnesses() and ProtocolDecl::getDefaultWitness().

That’s not sufficient though. We aren’t dealing with protocol requirements here, just default methods on a protocol.

For example in the following test, InheritedProtocol.hello() needs to inherit the documentation from BaseProtocol.hello but there is no protocol requirement that hello() can be a witness for. Any suggestion of how to implement that without my implementation?

protocol BaseProtocol {} extension BaseProtocol { /// Say hello func hello() { } } protocol InheritedProtocol: BaseProtocol {} extension InheritedProtocol { func hello() { } }

For example when we have ```swift protocol BaseProtocol {} extension BaseProtocol { /// Say hello func hello() { } } struct MyStruct: BaseProtocol { func hello() {} } ``` Then `MyStruct.hello` should inherit the doc comment from the implementation of `BaseProtocol.hello`. Currently no doc comment is associated with `MyStruct.hello`. rdar://126240546

ahoppen · 2024-08-03T06:41:32Z

@swift-ci Please smoke test Linux

ahoppen requested review from bnbarham, hamishknight, rintaro, hborla, slavapestov and xedin as code owners April 16, 2024 21:16

bnbarham approved these changes Apr 16, 2024

View reviewed changes

ahoppen force-pushed the inherit-doc-comment-from-protocol-default-impl branch from 619a27a to bdfe29f Compare April 17, 2024 20:59

hamishknight reviewed Apr 17, 2024

View reviewed changes

ahoppen force-pushed the inherit-doc-comment-from-protocol-default-impl branch from bdfe29f to a174859 Compare April 18, 2024 03:40

ahoppen requested a review from QuietMisdreavus as a code owner April 18, 2024 03:40

ahoppen commented Apr 18, 2024

View reviewed changes

hamishknight reviewed Apr 18, 2024

View reviewed changes

ahoppen force-pushed the inherit-doc-comment-from-protocol-default-impl branch from a174859 to 0795102 Compare April 18, 2024 22:45

ahoppen force-pushed the inherit-doc-comment-from-protocol-default-impl branch from 0795102 to d2b3942 Compare April 19, 2024 05:19

slavapestov requested changes Apr 19, 2024

View reviewed changes

ahoppen force-pushed the inherit-doc-comment-from-protocol-default-impl branch from d2b3942 to eec3d75 Compare August 3, 2024 00:28

ahoppen mentioned this pull request Aug 3, 2024

[IDE] Call into matchWitnessStructure to check if we should inherit a comment from an overridden declaration #75666

Merged

ahoppen merged commit 45e9534 into swiftlang:main Aug 7, 2024
3 checks passed

ahoppen deleted the inherit-doc-comment-from-protocol-default-impl branch August 7, 2024 17:01

	/// Check if there is an inherited protocol that has a default implementaiton
	/// Check if there is an inherited protocol that has a default implementation

		NominalTypeDecl *nominalType =
		dyn_cast_or_null<NominalTypeDecl>(VD->getDeclContext()->getAsDecl());

		// `RemoteP.P` also has an extension with a default implementation for `extraFunc` that does have
		// docs, but overriding it here should prevent those from appearing

		if (!memberComparisonTy->matches(vdComparisonTy, TypeMatchFlags::AllowOverride)) {
		continue;

Inherit doc comments from default implementations of inherited protocols #73066

Inherit doc comments from default implementations of inherited protocols #73066

Uh oh!

Conversation

ahoppen commented Apr 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Apr 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ahoppen Apr 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Apr 18, 2024

Uh oh!

ahoppen commented Apr 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Aug 3, 2024

Uh oh!

Uh oh!

Uh oh!

ahoppen commented Apr 16, 2024 •

edited

Loading

ahoppen Apr 18, 2024 •

edited

Loading