Skip to content

Conversation

d-ronnqvist
Copy link
Contributor

@d-ronnqvist d-ronnqvist commented Dec 4, 2023

Bug/issue #, if applicable:

Summary

This surface both the external "argument name" and the internal "parameter name" for FunctionParameter values.

The JSON key used for the internal name is "internalName", based on what the Swift symbol graph extractor emits.

With two different names to pick from, this updates the name property to behave as its documented:

The name of the function parameter, as referred to in user code (must match the name used in the documentation comment).

Since it's the internal name that's used in user code and in the documentation comment, name returns internalName.

If the parameter has both an external "argument name" and an internal "parameter name", then name returns the internal "parameter name" and externalName returns the external "argument name".

Dependencies

None

Testing

  • Extract a symbol graph file for

    public func doSomething(with someValue: Int) { }
  • Decode that symbol graph file.

    • The function's parameter's name should be "someValue"
    • The function's parameter's externalName should be "with"

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

  • Added tests
  • Ran the ./bin/test script and it succeeded
  • Updated documentation if necessary

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

/// The external argument name of the function parameter.
public var externalName: String

/// The internal parameter name of the function parameter.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the smallest of nits, but it kind of irks me that we surface the idea of "internal" versus "external" parameter names so much, since outside of Swift and Objective-C (as far as i know) the concept doesn't really apply. Is there a way we could structure the properties and/or documentation comments to make it clearer which version is the "usual" name? For example:

Suggested change
/// The internal parameter name of the function parameter.
/// The name of the function parameter used inside the function, if different from
/// `externalName`.
///
/// If no internal name was provided during initialization or from JSON, this parameter will
/// return `externalName` upon access.
///
/// > Important: This name must match the name given in the function's documentation comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the API to name and externalName and updated both their documentation comments in ead9b31.

How do you feel about that documentation?

case declarationFragments
case children
}

/// The name of the symbol, as referred to in user code (must match the name used in the documentation comment).
public var name: String // should we differentiate between internal and external names?
/// The name of the function parameter, as referred to in user code (must match the name used in the documentation comment).
Copy link
Contributor

Choose a reason for hiding this comment

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

While we're updating this doc comment: What does "user code" mean here? Going from the implementation, that's "the code inside the function", which actually isn't what i expected upon first read. Going just from API design, i think i'd prefer that this property remains the stored property of the "external name" and the "must match documentation comment" blurb be moved to the new internalName property. That way, the data structure names match the expected JSON, and the desired behavior is more closely documented since internalName already defers to externalName when no internal name is available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that would be confusing for developers using this API. In my opinion the name that developers mean when they talk about a parameters "name" is the internal name. For example, this documentation comment

/// Does something
/// - Parameters:
///   - first: Some number
///   - second: Some other number

could apply to either this Swift function

public func doSomething(with first: Int, and second: Int) {}

this C function

void doSomething(int first, int second);

or this Objective-C method

- (void)doSomethingWith:(NSInteger)first and:(NSInteger)second;

C and Objective-C doesn't have separate external parameter names. In both those cases the name are simply "first", and "second".

I started looking into this because I was surprised that for that Swift declaration the function parameter JSON was

{
  "name": "with",
  "internalName": "first",
  "declarationFragments": [
    ...
  ]
}

and

{
  "name": "and",
  "internalName": "second",
  "declarationFragments": [
    ...
  ]
}

To me it would be more consistent with other languages if it was flipped so that the "name" was the internal name and the external name was encoded as "externalName".

Since the Swift symbol graph generator had already emitted JSON like this for ~4 years I made this change in the SymbolKit decoding instead but if you prefer I can make that change in the Swift symbol graph generator and update the decoding here to look for "name" and "externalName" as the JSON keys.

Regardless of how the JSON looks, I feel like the expected behavior is that name always refers to the "internal name" and that in languages that have an explicit external name that's different from the internal name, developers can access that via externalName.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could also change this API to only include name: String and externalName: String? if that's less confusing (and still do switch during JSON decoding)

Copy link
Contributor Author

@d-ronnqvist d-ronnqvist Dec 6, 2023

Choose a reason for hiding this comment

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

Thinking about this API some more I think that that is the less confusing API to work with.

I made that change in ead9b31

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist d-ronnqvist merged commit 7d88602 into swiftlang:main Feb 1, 2024
@d-ronnqvist d-ronnqvist deleted the function-signature-internal-param branch February 1, 2024 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants