Skip to content
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.

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

Improve heuristic for extending return value with "on failure" info #1096

Merged

Conversation

d-ronnqvist
Copy link
Contributor

@d-ronnqvist d-ronnqvist commented Nov 13, 2024

Bug/issue #, if applicable: rdar://139465971

Summary

This improves the heuristic for when DocC extends authored return value documentation with "On failure, ..." information for Objective-C API that corresponds to Swift API that throws errors.

Dependencies

None.

Testing

  • In a mixed Swift and Objective-C project, add a Swift method that returns a value and throws errors.

    @objc
    func doSomething(with someValue: String) throws -> String { someValue }
    
  • Give a basic description of the return value.

    /// Returns: Some value.
    
  • Preview the documentation and navigate to the Objective-C representation of that method.

    • The return value documentation should include: "On failure, this method returns nil."
  • Rephrase the return value documentation to something like:

    • "Some value. If an error occurs, this function doesn't return a value."
    • "Some value. On failure, this function doesn't return a value."
    • "Some value. If doing something fails, this function doesn't return a value."
    • "Some value. If something unexpected happens, this function returns nil instead."
    • "Some value, or nil if something goes wrong."
  • Preview the documentation again and navigate to the Objective-C representation of that method.

    • The return value documentation shouldn't include: "On failure, this method returns nil."

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 d-ronnqvist requested a review from anferbui November 13, 2024 17:30
…about Objective-C return values for bridged API

rdar://139432674
Copy link
Contributor

@anferbui anferbui left a comment

Choose a reason for hiding this comment

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

Overall it's looking good, there's some small typos and I've got a question on the implementation of the pattern matching.

@d-ronnqvist d-ronnqvist force-pushed the error-return-value-false-positive branch from 5fdbf78 to 40724b1 Compare December 19, 2024 15:51
…d return value documentation.

Also, rephrase note as tip
@d-ronnqvist d-ronnqvist force-pushed the error-return-value-false-positive branch from 40724b1 to d5aea7f Compare December 19, 2024 15:53
@d-ronnqvist d-ronnqvist requested a review from anferbui December 19, 2024 15:54
@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist d-ronnqvist dismissed anferbui’s stale review December 19, 2024 15:55

The requested changes have been made

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

Copy link
Contributor

@anferbui anferbui left a comment

Choose a reason for hiding this comment

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

The documentation looks great.

I left a comment about using Regex instead of NSRegularExpression, but I don't think that should block merge if you don't think it's relevant.

/// These words only match at word boundaries or when surrounded by single backticks ("`").
///
/// This is used as a heuristic to give an indication if the return value documentation possibly documents what happens when an error occurs.
private let returnValueDescribesErrorRegex = try! NSRegularExpression(pattern: "(\\b|`)(error|fail(ure)?s?|nil|null)(\\b|`)", options: .caseInsensitive)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use Regex instead? It has an option for case insensitivity as well 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regex is available as of macOS 13 and we currently support macOS 12 (bumped very recently).

We probably could bump to macOS 13 but I didn't do that for this fix because this regular expression wasn't all that complicated.

@d-ronnqvist
Copy link
Contributor Author

I left a comment about using Regex instead of NSRegularExpression, but I don't think that should block merge if you don't think it's relevant.

I think it's relevant and it would make the code more readable but it would require bumping the supported platform versions.

Still, I think that it's a topic that's worth revisiting in a separate PR. Then we could update all the regular expressions at the same time.

@d-ronnqvist d-ronnqvist merged commit 2336b82 into swiftlang:main Dec 20, 2024
2 checks passed
@d-ronnqvist d-ronnqvist deleted the error-return-value-false-positive branch December 20, 2024 12:29
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.

2 participants