Skip to content

Conversation

xedin
Copy link
Contributor

@xedin xedin commented Nov 16, 2022

getParameterAt needs to be more defensive about the input
because caller cannot always perform all the checks upfront.

Resolves: rdar://102085039

`getParameterAt` needs to be more defensive about the input
because caller cannot always perform all the checks upfront.

Resolves: rdar://102085039
@xedin xedin requested a review from hborla November 16, 2022 23:37
@xedin
Copy link
Contributor Author

xedin commented Nov 16, 2022

@swift-ci please smoke test

@slavapestov
Copy link
Contributor

I looked at the test case in a debugger and I think the caller is calling getParameterAt() with a parameter index of 0 when in fact it's looking at the self parameter. So you have a function type (T) -> (Args...) -> U, and it's looking at the outer function type and not the inner one.

Can we detect this case there instead of adding this defensive check?

Otherwise, if the function does in fact have 1 parameter, getParameterAt() will succeed, but it might trigger incorrect behavior because it's looking at 'self' and it thinks it's looking at the first actual parameter.

@slavapestov
Copy link
Contributor

I see we compute the callee as follows:

  ValueDecl *callee = nullptr;
  bool appliedSelf = false;

  // Resolve the callee for the application.
  auto *calleeLocator = cs.getCalleeLocator(loc);
  if (auto overload = cs.findSelectedOverloadFor(calleeLocator)) {
    callee = overload->choice.getDeclOrNull();
    appliedSelf = hasAppliedSelf(cs, overload->choice);
  }

Maybe hasAppliedSelf is what we want? Just a wild guess!

@xedin
Copy link
Contributor Author

xedin commented Nov 17, 2022

@slavapestov I don't think so, it's crashing in simplifyApplicableFnConstraint which has an applied reference, it tries to fetch parameter 0 (because there is an argument) from failure() where ParameterList * is empty.

@slavapestov
Copy link
Contributor

slavapestov commented Nov 17, 2022

It's calling shouldOpenExistentialCallArgument() inside this for loop in matchCallArguments():

    for (auto argIdx : parameterBindings[paramIdx]) {

So it thinks there's at least one parameter in the call. Here are the two function types:

(lldb) print type1.dump()
(function_type escaping
  (input=function_params num_params=1
    (param
      (existential_type
        (protocol_type decl=Swift.(file).Error))))
  (output=type_variable_type id=20))
(lldb) print type2.dump()
(function_type escaping
  (input=function_params num_params=1
    (param
      (bound_generic_enum_type decl=Swift.(file).Result
        (type_variable_type id=16)
        (type_variable_type id=17))))
  (output=function_type escaping
    (input=function_params num_params=0)
    (output=optional_type
      (type_variable_type id=17))))

type2 is the opened type of the decl. It appears to be looking at the outer application of self.

@slavapestov
Copy link
Contributor

I guess hasAppliedSelf doesn't give us enough information, it tells us that the self clause exists but we don't know if we're matching the outer function type with the self parameter, or the inner function type with the formal parameters to our method. Maybe looking at the locator can determine this?

@slavapestov
Copy link
Contributor

What if you change shouldOpenExistentialCallArgument() to look into the ParameterListInfo instead of getting the ParameterList from the callee directly? It looks like we already have to handle the same situation with default arguments (imagine the first formal argument is defaulted, but we're matching the self parameter). Opening an existential should similarly consult the ParameterListInfo. It looks like the ParameterListInfo::ParameterListInfo() constructor already has the right logic to detect if we're matching the self parameter and skip most of the logic.

@xedin
Copy link
Contributor Author

xedin commented Nov 17, 2022

I can look into changing shouldOpenExistentialCallArgument once I'm done with other things, I think it's good to have getParameterAt be more defensive still.

@xedin
Copy link
Contributor Author

xedin commented Nov 17, 2022

@swift-ci please smoke test Linux platform

@xedin
Copy link
Contributor Author

xedin commented Nov 17, 2022

@swift-ci please smoke test macOS platform

@xedin
Copy link
Contributor Author

xedin commented Nov 17, 2022

@swift-ci please test

@xedin xedin merged commit c17d35d into swiftlang:main Nov 28, 2022
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