Add support for parameter names #136

fdiaz · 2022-09-16T15:45:22Z

This PR adds support for parameter names in functions.

We were already supporting argument labels so adding this was fairly trivial.

codecov · 2022-09-16T16:07:48Z

Codecov Report

Merging #136 (2d9a713) into main (53aec4d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 2d9a713 differs from pull request most recent head f4c285d. Consider uploading reports for the commit f4c285d to get more accurate results

@@           Coverage Diff           @@
##             main     #136   +/-   ##
=======================================
  Coverage   98.82%   98.83%           
=======================================
  Files          40       40           
  Lines        1536     1539    +3     
=======================================
+ Hits         1518     1521    +3     
  Misses         18       18

Impacted Files	Coverage Δ
...InspectorVisitors/FunctionDeclarationVisitor.swift	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

shepting

LGTM.

dfed

LGTM with a minor nit

dfed · 2022-09-16T20:19:20Z

Sources/SwiftInspectorVisitors/FunctionDeclarationVisitor.swift

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


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?

I do think the comment By default, parameters use their parameter name as their argument label is correct per this

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

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!

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)

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.

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.

bachand · 2022-09-16T21:35:00Z

Sources/SwiftInspectorVisitors/FunctionDeclarationVisitor.swift

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


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.

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

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.

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.

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.

Ok, this is what I ended up with f4c285d

bachand · 2022-09-16T21:37:14Z

Sources/SwiftInspectorVisitors/Tests/FunctionDeclarationVisitorSpec.swift

@@ -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") {


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

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?

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

bachand · 2022-09-16T21:37:58Z

Sources/SwiftInspectorVisitors/FunctionDeclarationVisitor.swift

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

  public struct ArgumentInfo: Codable, Hashable {
    public let argumentLabelName: String
+    public let parameterName: String


I was going to say that we should make the argument label optional but since Swift stresses both exist I think this is fine.

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

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

fdiaz · 2022-09-19T20:22:17Z

I'm gonna merge for now but do leave more feedback if you have it and we can address it later!

Add support for parameter names

2d9a713

shepting approved these changes Sep 16, 2022

View reviewed changes

dfed approved these changes Sep 16, 2022

View reviewed changes

bachand approved these changes Sep 16, 2022

View reviewed changes

Clarify parameter name and argument label

f4c285d

fdiaz merged commit d0d4dd1 into main Sep 19, 2022

fdiaz deleted the support-parameter-names branch September 19, 2022 20:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for parameter names #136

Add support for parameter names #136

fdiaz commented Sep 16, 2022

codecov bot commented Sep 16, 2022 •

edited

shepting left a comment

dfed left a comment

dfed Sep 16, 2022

bachand Sep 16, 2022

dfed Sep 16, 2022

fdiaz Sep 19, 2022

dfed Sep 19, 2022

fdiaz Sep 19, 2022

dfed Sep 19, 2022 •

edited

bachand Sep 16, 2022

bachand Sep 16, 2022

fdiaz Sep 19, 2022

fdiaz Sep 19, 2022 •

edited

fdiaz Sep 19, 2022

fdiaz Sep 19, 2022

bachand Sep 16, 2022

fdiaz Sep 19, 2022

fdiaz Sep 19, 2022

bachand Sep 16, 2022

dfed Sep 16, 2022

fdiaz Sep 19, 2022

fdiaz commented Sep 19, 2022

Add support for parameter names #136

Add support for parameter names #136

Conversation

fdiaz commented Sep 16, 2022

codecov bot commented Sep 16, 2022 • edited

Codecov Report

shepting left a comment

Choose a reason for hiding this comment

dfed left a 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

dfed Sep 19, 2022 • edited

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

fdiaz Sep 19, 2022 • edited

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

fdiaz commented Sep 19, 2022

codecov bot commented Sep 16, 2022 •

edited

dfed Sep 19, 2022 •

edited

fdiaz Sep 19, 2022 •

edited