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

[CodeCompletion][Sema] Allow missing args when solving if the completion location indicates the user may intend to write them later. #34337

Conversation

nathawes
Copy link
Contributor

@nathawes nathawes commented Oct 16, 2020

Int and String members should both be prioritized for the completion below despite the missing argument for the first overload since the second argument may just have not been written yet:

func foo(a: Int, b: Int) {}
func foo(a: String) {}

foo(a: someBase.<complete here>

Similarly, members of type String should also be prioritized for the completion below (rather than just Int) since the user may intend to use $1 later in the expression.

func bar(a: (Int) -> ()) {}
func bar(a: (String, Int) -> ()) {}

bar { $0.<complete here> }

@nathawes nathawes requested review from xedin and rintaro October 16, 2020 21:12
@nathawes
Copy link
Contributor Author

@swift-ci please test

@@ -1135,6 +1175,28 @@ class ArgumentFailureTracker : public MatchCallArgumentListener {
getExtraneousArguments() const {
return ExtraArguments;
}

private:
bool areMissingArgsInvalid(ConstraintLocator *locator) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should convert this into a filter over MissingArguments in argument tracker instead and only record the ones which come before the token?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or even better - ignore missing argument recorded if it comes after the token that way destructor of the ArgumentFailureTracker could stay untouched?

SourceRange range = closure->getSourceRange();
if (range.isValid() && SM.rangeContainsCodeCompletionLoc(range)) {
shouldFix = !closure->hasAnonymousClosureVars() &&
(!args.empty() || closure->getInLoc().isValid());
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe an early return here in reverse situation instead?

Copy link
Contributor Author

@nathawes nathawes Oct 16, 2020

Choose a reason for hiding this comment

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

Would missing the single tuple argument handling below (need to expand a few lines) be a problem?
Actually I guess these cases should be exclusive.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think that should be okay.

Copy link
Contributor

@xedin xedin left a comment

Choose a reason for hiding this comment

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

Overall LGTM, I have left a couple of comments inline.

@nathawes nathawes force-pushed the allow-missing-args-when-type-checking-for-completion branch from 36f89a0 to b8a4af2 Compare October 17, 2020 00:03
@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - b8a4af2264418a5b3d53d0307f37dd090018392f

@nathawes nathawes force-pushed the allow-missing-args-when-type-checking-for-completion branch from b8a4af2 to cd6f200 Compare October 17, 2020 02:01
Comment on lines +1181 to +1185
struct SynthesizedArg {
unsigned paramIdx;
AnyFunctionType::Param param;
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is no longer necessary (in my original changes it also tracked the index to insert the missing arg at) but I left it in for the slight readability improvement over a std::pair (e.g. paramIdx vs first)

Copy link
Contributor

Choose a reason for hiding this comment

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

That works for me, thank you!

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - b8a4af2264418a5b3d53d0307f37dd090018392f

@nathawes
Copy link
Contributor Author

@swift-ci please test

…ion location indicates the user may intend to write them later.

func foo(a: Int, b: Int) {}
func foo(a: String) {}

// Int and String should both be valid, despite the missing argument for the
// first overload since the second arg may just have not been written yet.
foo(a: <complete here>

func bar(a: (Int) -> ()) {}
func bar(a: (String, Int) -> ()) {}

// $0 being of type String should be valid, rather than just Int, since $1 may
// just have not been written yet.
bar { $0.<complete here> }
@nathawes nathawes force-pushed the allow-missing-args-when-type-checking-for-completion branch from cd6f200 to 15f5222 Compare October 19, 2020 19:16
@nathawes
Copy link
Contributor Author

@swift-ci please test and merge

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - cd6f200fefa7342261da6181ec8c8ee71baba7bc

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - cd6f200fefa7342261da6181ec8c8ee71baba7bc

@swift-ci swift-ci merged commit b654008 into swiftlang:main Oct 19, 2020
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.

4 participants