Skip to content

Inherit doc comments from default implementations of inherited protocols #73066

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 1 commit into from
Aug 7, 2024
Merged

Inherit doc comments from default implementations of inherited protocols #73066

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
51 changes: 38 additions & 13 deletions lib/AST/DocComment.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,24 +427,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) {
Copy link
Contributor

@slavapestov slavapestov Apr 19, 2024

Choose a reason for hiding this comment

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

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

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

Copy link
Member Author

@ahoppen ahoppen Apr 24, 2024

Choose a reason for hiding this comment

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

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() { }
}

// Only applies to protocol extension member.
auto *protocol = VD->getDeclContext()->getExtendedProtocolDecl();
if (!protocol)
NominalTypeDecl *nominalType =
dyn_cast_or_null<NominalTypeDecl>(VD->getDeclContext()->getAsDecl());
Comment on lines +433 to +434
Copy link
Contributor

@hamishknight hamishknight Apr 17, 2024

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@ahoppen ahoppen Apr 18, 2024

Choose a reason for hiding this comment

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

We do with the lines below, right?

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

Copy link
Contributor

@hamishknight hamishknight Apr 18, 2024

Choose a reason for hiding this comment

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

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

if (!nominalType) {
nominalType = VD->getDeclContext()->getExtendedProtocolDecl();
}
if (!nominalType)
return std::nullopt;

SmallVector<ValueDecl *, 2> members;
protocol->lookupQualified(const_cast<ProtocolDecl *>(protocol),
DeclNameRef(VD->getName()),
VD->getLoc(), NLOptions::NL_ProtocolMembers,
members);
nominalType->lookupQualified(nominalType, DeclNameRef(VD->getName()),
VD->getLoc(), NLOptions::NL_ProtocolMembers,
members);
Comment on lines +442 to +444
Copy link
Contributor

@hamishknight hamishknight Apr 18, 2024

Choose a reason for hiding this comment

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

Thought of a couple more edge cases :)

  1. static mismatch
protocol P {}
extension P {
  /// hello
  static func foo() {}
}
struct S: P {
  func foo() {}
}
func bar(_ x: S) {
  x.foo()
}
  1. 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)
}
  1. 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
}

Copy link
Member Author

@ahoppen ahoppen Apr 18, 2024

Choose a reason for hiding this comment

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

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.


std::optional<ResultWithDecl> result;
for (auto *member : members) {
if (!isa<ProtocolDecl>(member->getDeclContext()) ||
!member->isProtocolRequirement())
continue;
Type vdComparisonTy = VD->getInterfaceType();
if (!vdComparisonTy) {
return std::nullopt;
}
if (auto fnTy = vdComparisonTy->getAs<AnyFunctionType>()) {
// Strip off the 'Self' parameter.
vdComparisonTy = fnTy->getResult();
}
Comment on lines +451 to +454
Copy link
Contributor

@hamishknight hamishknight Apr 18, 2024

Choose a reason for hiding this comment

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

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


for (auto *member : members) {
if (isa<AbstractFunctionDecl>(member) || isa<AbstractStorageDecl>(member)) {
if (VD->isStatic() != member->isStatic()) {
continue;
}
Type memberComparisonTy = member->getInterfaceType();
if (!memberComparisonTy) {
continue;
}
if (auto fnTy = memberComparisonTy->getAs<AnyFunctionType>()) {
// Strip off the 'Self' parameter.
memberComparisonTy = fnTy->getResult();
}
if (!vdComparisonTy->matches(memberComparisonTy, TypeMatchFlags::AllowOverride)) {
continue;
}
}
auto newResult = visit(member);
if (!newResult)
continue;
Expand Down Expand Up @@ -487,10 +512,10 @@ class CommentProviderFinder final {
if (auto result = findOverriddenDecl(VD))
return result;

if (auto result = findDefaultProvidedDecl(VD))
if (auto result = findRequirementDecl(VD))
return result;

if (auto result = findRequirementDecl(VD))
if (auto result = findDefaultProvidedDecl(VD))
return result;

return std::nullopt;
Expand Down
157 changes: 157 additions & 0 deletions test/SourceKit/CursorInfo/cursor_inherit_doc_comment.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,157 @@
protocol BaseProtocol {}

extension BaseProtocol {
/// Say hello
func hello() { }
}

protocol InheritedProtocol: BaseProtocol {}

extension InheritedProtocol {
func hello() { }
}

func testInheritBetweenExtensions(x: MyStruct) {
struct MyStruct: InheritedProtocol {}

// RUN: %sourcekitd-test -req=cursor -req-opts=retrieve_symbol_graph=1 -pos=%(line + 1):14 %s -- %s | %FileCheck %s --check-prefix BETWEEN_EXTENSIONS
MyStruct().hello()
// BETWEEN_EXTENSIONS: DOC COMMENT
// BETWEEN_EXTENSIONS-NEXT: Say hello
// BETWEEN_EXTENSIONS-NEXT: DOC COMMENT XML
// BETWEEN_EXTENSIONS: SYMBOL GRAPH BEGIN
// BETWEEN_EXTENSIONS: Say hello
// BETWEEN_EXTENSIONS: SYMBOL GRAPH END
}

func testInheritFromExtensionToStruct(x: MyStruct) {
struct MyStruct: BaseProtocol {
func hello() {}
}

// RUN: %sourcekitd-test -req=cursor -req-opts=retrieve_symbol_graph=1 -pos=%(line + 1):14 %s -- %s | %FileCheck %s --check-prefix EXTENSION_TO_STRUCT
MyStruct().hello()
// EXTENSION_TO_STRUCT: DOC COMMENT
// EXTENSION_TO_STRUCT-NEXT: Say hello
// EXTENSION_TO_STRUCT-NEXT: DOC COMMENT XML
// EXTENSION_TO_STRUCT: SYMBOL GRAPH BEGIN
// EXTENSION_TO_STRUCT: Say hello
// EXTENSION_TO_STRUCT: SYMBOL GRAPH END
}

protocol ProtocolWithDefaultedRequirement {
/// Doc for the requirement
func hello()
}

extension ProtocolWithDefaultedRequirement {
/// Doc for the default implementation
func hello() {}
}

func testProtocolWithDefaultedRequirement(foo: any ProtocolWithDefaultedRequirement) {
// RUN: %sourcekitd-test -req=cursor -req-opts=retrieve_symbol_graph=1 -pos=%(line + 1):7 %s -- %s | %FileCheck %s --check-prefix DEFAULTED_REQUIREMENT
foo.hello()
// DEFAULTED_REQUIREMENT: DOC COMMENT
// DEFAULTED_REQUIREMENT-NEXT: Doc for the requirement
// DEFAULTED_REQUIREMENT-NEXT: DOC COMMENT XML
// DEFAULTED_REQUIREMENT: SYMBOL GRAPH BEGIN
// DEFAULTED_REQUIREMENT: Doc for the requirement
// DEFAULTED_REQUIREMENT: SYMBOL GRAPH END
}


func testDontInheritFromFunctionWithDifferentType() {
struct MyStruct: InheritedProtocol {
func hello() -> String { return "Hello" }
}
// RUN: %sourcekitd-test -req=cursor -pos=%(line + 1):30 %s -- %s | %FileCheck %s --check-prefix FROM_FUNCTION_WITH_DIFFERENT_TYPE
let x: String = MyStruct().hello()
// FROM_FUNCTION_WITH_DIFFERENT_TYPE: DOC COMMENT
// FROM_FUNCTION_WITH_DIFFERENT_TYPE-NEXT: DOC COMMENT XML
}

protocol ProtocolWithThrowingFunction {
/// Say hello
func hello() throws
}

func testInheritEvenIfThrowingNonThrowingMismatches() {
struct MyStruct: ProtocolWithThrowingFunction {
func hello() { }
}
// RUN: %sourcekitd-test -req=cursor -pos=%(line + 1):14 %s -- %s | %FileCheck %s --check-prefix SATISFY_THROWING_WITH_NON_THROWING
MyStruct().hello()
// SATISFY_THROWING_WITH_NON_THROWING: DOC COMMENT
// SATISFY_THROWING_WITH_NON_THROWING-NEXT: Say hello
// SATISFY_THROWING_WITH_NON_THROWING-NEXT: DOC COMMENT XML
}

protocol ProtocolWithAccessor {
/// The greeting
var greeting: String { get }
}

func testSatisfyAccessorRequirement() {
struct MyStruct: ProtocolWithAccessor {
var greeting: String = "hello"
}
// RUN: %sourcekitd-test -req=cursor -pos=%(line + 1):14 %s -- %s | %FileCheck %s --check-prefix ACCESSOR_REQUIREMENT
MyStruct().greeting
// ACCESSOR_REQUIREMENT: DOC COMMENT
// ACCESSOR_REQUIREMENT-NEXT: The greeting
// ACCESSOR_REQUIREMENT-NEXT: DOC COMMENT XML
}

protocol ProtocolWithMethodReturningOptional {}
extension ProtocolWithMethodReturningOptional {
/// hello
func hello() -> String? { nil }
}

func testInheritFromLessSpecificOverridden() {
struct MyStruct: ProtocolWithMethodReturningOptional {
func hello() -> String { "hello" }
}

// RUN: %sourcekitd-test -req=cursor -pos=%(line + 1):14 %s -- %s | %FileCheck %s --check-prefix INHERIT_FROM_LESS_SPECIFIC_OVERRIDDEN
MyStruct().hello()
// INHERIT_FROM_LESS_SPECIFIC_OVERRIDDEN: DOC COMMENT
// INHERIT_FROM_LESS_SPECIFIC_OVERRIDDEN-NEXT: hello
// INHERIT_FROM_LESS_SPECIFIC_OVERRIDDEN-NEXT: DOC COMMENT XML
}


protocol ProtocolWithStaticMethod {}
extension ProtocolWithStaticMethod {
/// hello
static func hello() {}
}

func testDontInheritFromStaticToNonStatic() {
struct MyStruct: ProtocolWithStaticMethod {
func hello() {}
}

// RUN: %sourcekitd-test -req=cursor -pos=%(line + 1):14 %s -- %s | %FileCheck %s --check-prefix DONT_INHERIT_FROM_STATIC_TO_NON_STATIC
MyStruct().hello()
// DONT_INHERIT_FROM_STATIC_TO_NON_STATIC: DOC COMMENT
// DONT_INHERIT_FROM_STATIC_TO_NON_STATIC-NEXT: DOC COMMENT XML
}

protocol ProtocolWithStaticMethodReturningSelf {
/// hello
static func hello(_ greeting: String) -> Self {}
}

func testInheritStaticFuncToEnumCase() {
enum MyEnum: ProtocolWithStaticMethodReturningSelf {
case hello(String)
}

// RUN: %sourcekitd-test -req=cursor -pos=%(line + 1):10 %s -- %s | %FileCheck %s --check-prefix INHERIT_FROM_STATIC_FUNC_TO_ENUM_CASE
MyEnum.hello
// INHERIT_FROM_STATIC_FUNC_TO_ENUM_CASE: DOC COMMENT
// INHERIT_FROM_STATIC_FUNC_TO_ENUM_CASE-NEXT: hello
// INHERIT_FROM_STATIC_FUNC_TO_ENUM_CASE-NEXT: DOC COMMENT XML
}
10 changes: 1 addition & 9 deletions test/SymbolGraph/Relationships/Synthesized/RemoteInheritedDocs.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
// RUN: %FileCheck %s --input-file %t/RemoteInheritedDocs.symbols.json --check-prefix BONUS
// RUN: %FileCheck %s --input-file %t/RemoteInheritedDocs.symbols.json --check-prefix INHERIT
// RUN: %FileCheck %s --input-file %t/RemoteInheritedDocs.symbols.json --check-prefix LOCAL
// RUN: %FileCheck %s --input-file %t/RemoteInheritedDocs.symbols.json --check-prefix OVERRIDE
// RUN: %FileCheck %s --input-file %t/[email protected] --check-prefix EXTENSION

// RUN: %target-swift-symbolgraph-extract -module-name RemoteInheritedDocs -I %t -pretty-print -output-dir %t -skip-inherited-docs
Expand All @@ -17,12 +16,11 @@
// RUN: %FileCheck %s --input-file %t/RemoteInheritedDocs.symbols.json --check-prefix BONUS
// RUN: %FileCheck %s --input-file %t/RemoteInheritedDocs.symbols.json --check-prefix SKIP
// RUN: %FileCheck %s --input-file %t/RemoteInheritedDocs.symbols.json --check-prefix LOCAL
// RUN: %FileCheck %s --input-file %t/RemoteInheritedDocs.symbols.json --check-prefix OVERRIDE

// SOME: "source": "s:19RemoteInheritedDocs1SV8someFuncyyF"
// SOME-NEXT: "target": "s:19RemoteInheritedDocs1SV"
// SOME-NEXT: "sourceOrigin"
// SOME-NEXT: "identifier": "s:7RemoteP1PP8someFuncyyF"
// SOME-NEXT: "identifier": "s:7RemoteP1PP0A13InheritedDocsE8someFuncyyF"
// SOME-NEXT: "displayName": "P.someFunc()"

// OTHER: "source": "s:19RemoteInheritedDocs1SV9otherFuncyyF"
Expand All @@ -42,9 +40,6 @@

// LOCAL: Local docs override!

// OVERRIDE-NOT: Extra default docs!
// OVERRIDE-NOT: Extension override!

// EXTENSION-NOT: Some Protocol

import RemoteP
Expand All @@ -53,9 +48,6 @@ import RemoteP
// but `otherFunc` does. Regardless, each one needs a `sourceOrigin` field connecting it to
// upstream.

// `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

Comment on lines -56 to -58
Copy link
Member Author

@ahoppen ahoppen Apr 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

@QuietMisdreavus QuietMisdreavus Apr 18, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@ahoppen ahoppen Apr 18, 2024

Choose a reason for hiding this comment

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

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

// When emitting extension block symbols, local extension blocks should never inherit documentation
// from the original type declaration.

Expand Down