Skip to content
This repository has been archived by the owner on Sep 20, 2022. It is now read-only.

Add support for parameter names #136

Merged
merged 2 commits into from Sep 19, 2022
Merged

Add support for parameter names #136

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2d9a713
Add support for parameter names
fdiaz Sep 16, 2022
f4c285d
Clarify parameter name and argument label
fdiaz Sep 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 8 additions & 3 deletions Sources/SwiftInspectorVisitors/FunctionDeclarationVisitor.swift
View file
Open in desktop
Expand Up @@ -56,17 +56,21 @@ fileprivate final class FunctionSignatureVisitor: SyntaxVisitor {
let type = node.type?.typeDescription
else { return .skipChildren }

appendArgument(argumentLabelName: argumentLabelName, type: type)
let parameterName = node.secondName?.text

appendArgument(argumentLabelName: argumentLabelName, parameterName: parameterName, type: type)
return .skipChildren
}
override func visit(_ node: ReturnClauseSyntax) -> SyntaxVisitorContinueKind {
returnType = node.returnType.typeDescription
return .skipChildren
}

fileprivate func appendArgument(argumentLabelName: String, type: TypeDescription) {
fileprivate func appendArgument(argumentLabelName: String, parameterName: String?, type: TypeDescription) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Technically speaking if a function argument only has a parameter name and no argument label then the single name is the parameter name. The way we've written this function though the single name would be the argument label.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we rework this function so that the argument label name is optional and the parameter name is required?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Oh yeah, I think I got this the other way around. I disagree with making these optional though, that seems incorrect since we'd never really have a nil argument label.

Copy link
Owner Author

@fdiaz fdiaz Sep 19, 2022 •
edited

Choose a reason for hiding this comment

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

I went back and re-read this.

Based on:

Each function parameter has both an argument label and a parameter name. The argument label is used when calling the function; each argument is written in the function call with its argument label before it. The parameter name is used in the implementation of the function.

I'm again confused.

Copy link
Owner Author

Choose a reason for hiding this comment

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

So I don't think

Technically speaking if a function argument only has a parameter name and no argument label then the single name is the parameter name

Is correct?

How I understand this is that:

  • A parameter has 2 members that always exist an argument label and a name (i.e. the "parameter name")
  • When we explicitly define only one "member" in the parameter, then the argument label is equal to the name.

So I think making either of these nil is just incorrect, since they can never technically be nil.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ok, this is what I ended up with f4c285d

var arguments = self.arguments ?? []
arguments.append(.init(argumentLabelName: argumentLabelName, type: type))
// By default, parameters use their parameter name as their argument label
let param = parameterName ?? argumentLabelName
arguments.append(.init(argumentLabelName: argumentLabelName, parameterName: param, type: type))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't love the name param. I might write this as:

Suggested change
// By default, parameters use their parameter name as their argument label
let param = parameterName ?? argumentLabelName
arguments.append(.init(argumentLabelName: argumentLabelName, parameterName: param, type: type))
// By default, parameters use their parameter name as their argument label
let param = parameterName ?? argumentLabelName
arguments.append(.init(
argumentLabelName: argumentLabelName,
parameterName: parameterName ?? argumentLabelName,
type: type))

As an aside, the comment By default, parameters use their parameter name as their argument label doesn't make a whole lot of sense. The parameter name only falls back to the argument label if there is no explicit parameter name, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I do think the comment By default, parameters use their parameter name as their argument label is correct per this
Screen Shot 2022-09-16 at 2 33 03 PM

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's helpful context! Within that context, the comment makes sense. Putting that comment on top of the assignment of param is what confused me: we're setting the parameter name in the next line, not the argument label. And we're setting the parameter name to be the same as the argument label, rather than setting the argument label to be the same as the parameter name which is what the comment suggests we should be doing.

Maybe what's confusing me more is the fact that we're setting the parameterName to be argumentLabelName if there is no explicit parameterName. In fact, doing this loses data.

This code would create identical output for the following scanned methods:

func doSomething(parameterName parameterName: Void)
func doSomething(parameterName: Void)

Ideally the output of our scans could be used to re-create the original code (or at least the method signatures on the scanned types). This implementation doesn't let us do that with confidence

Copy link
Owner Author

Choose a reason for hiding this comment

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

I don't love the name param. I might write this as:

I thought about doing that but wanted to take it out into its own line so I could add the code comment above. Since this is in the implementation and I think param pretty clearly refers to "parameter" I think this is fine? But lemme know if you disagree!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that the meaning of the param variable is clear. I still don't love it, but I don't consider it blocking at all.

I would recommend updating the comment though, and possibly updating the API here to ensure that the output types would be different for the following two methods:

func doSomething(parameterName parameterName: Void)
func doSomething(parameterName: Void)

Copy link
Owner Author

Choose a reason for hiding this comment

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

Maybe what's confusing me more is the fact that we're setting the parameterName to be argumentLabelName if there is no explicit parameterName. In fact, doing this loses data.

I don't see a problem with scanning those 2 equally since they are, in fact, equal.

I think we've decided to be opinionated here in terms of style (e.g. using Array vs []) so I think it's ok to lose the fact that the parameter name and argument labels are or are not explicitly defined.

Copy link
Collaborator

@dfed dfed Sep 19, 2022 •
edited

Choose a reason for hiding this comment

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

they're equal from a "how would you call the code" POV, but not from a "how would you generate a protocol for this method" POV. Though I just tried it out and the following code does compile:

protocol TestProtocol {
    func doSomething(parameterName parameterName: String)
}

final class TestConformance: TestProtocol {
    func doSomething(parameterName: String) {
        // do something
    }
}

as does

protocol TestProtocol {
    func doSomething(parameterName: String)
}

final class TestConformance: TestProtocol {
    func doSomething(parameterName parameterName: String) {
        // do something
    }
}

So I withdraw the objection, since this API is not limiting a consumer's ability to generate compiling code.

self.arguments = arguments
}

Expand All @@ -87,6 +91,7 @@ public struct FunctionDeclarationInfo: Codable, Hashable {

public struct ArgumentInfo: Codable, Hashable {
public let argumentLabelName: String
public let parameterName: String
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was going to say that we should make the argument label optional but since Swift stresses both exist I think this is fine.
Screen Shot 2022-09-16 at 2 37 30 PM

Copy link
Collaborator

Choose a reason for hiding this comment

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

Per my realization at the end of #136 (comment) I think making the parameterName optional would be helpful.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I just pushed a new commit that hopefully will help clarify a bit f4c285d

public let type: TypeDescription
}
}
16 changes: 13 additions & 3 deletions Sources/SwiftInspectorVisitors/Tests/FunctionDeclarationVisitorSpec.swift
View file
Open in desktop
Expand Up @@ -175,9 +175,12 @@ final class FunctionDeclarationVisitorSpec: QuickSpec {
try? self.sut.walkContent(content)
}

it("finds the parameter label") {
it("finds the argument label") {
expect(self.sut.functionDeclarations.first?.arguments?.first?.argumentLabelName) == "_"
}
it("finds the parameter name") {
expect(self.sut.functionDeclarations.first?.arguments?.first?.parameterName) == "string"
}
it("calculates the selectorName") {
expect(self.sut.functionDeclarations.first?.selectorName) == "print(_:)"
}
Expand All @@ -200,12 +203,15 @@ final class FunctionDeclarationVisitorSpec: QuickSpec {
it("calculates the selectorName") {
expect(self.sut.functionDeclarations.first?.selectorName) == "append(argumentLabelName:)"
}
it("finds the parameter name") {
expect(self.sut.functionDeclarations.first?.arguments?.first?.parameterName) == "parameterName"
}
}

context("function with multiple parameters") {
beforeEach {
let content = """
func append(argumentLabelName parameterName: String, type: TypeDescription) {
func append(argumentLabelName parameterName: String, type secondParameterName: TypeDescription) {
// ...
}
"""
Expand All @@ -216,10 +222,14 @@ final class FunctionDeclarationVisitorSpec: QuickSpec {
it("finds the correct number of parameters") {
expect(self.sut.functionDeclarations.first?.arguments?.count) == 2
}
it("finds the parameter labels") {
it("finds the argument labels") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add a test somewhere for a case where there is no argument label and only a parameter name?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I don't think that's possible, the argument label is always present IIUC?

Each function parameter has both an argument label and a parameter name. The argument label is used when calling the function; each argument is written in the function call with its argument label before it. The parameter name is used in the implementation of the function.

I think you're referring to having the argument label be the same as the parameter name, which I think we already have tests for?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I just pushed a new commit that hopefully will help clarify a bit f4c285d

expect(self.sut.functionDeclarations.first?.arguments?.first?.argumentLabelName) == "argumentLabelName"
expect(self.sut.functionDeclarations.first?.arguments?.last?.argumentLabelName) == "type"
}
it("finds the parameter names") {
expect(self.sut.functionDeclarations.first?.arguments?.first?.parameterName) == "parameterName"
expect(self.sut.functionDeclarations.first?.arguments?.last?.parameterName) == "secondParameterName"
}
it("calculates the selectorName") {
expect(self.sut.functionDeclarations.first?.selectorName) == "append(argumentLabelName:type:)"
}
Expand Down