Skip to content

[Sema] Fix for non-API level property wrappers with closure. #63149

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

Merged
merged 3 commits into from
Jan 26, 2023

Conversation

Vespen
Copy link
Contributor

@Vespen Vespen commented Jan 21, 2023

Hi,

I was using property wrappers in my code and encountered a compiler crash. Here is a code to reproduce it:

@propertyWrapper
struct Wrapper {
  let wrappedValue: Int
}

class Class {
  static func staticMethod(@Wrapper value: Int) {
  }
}

func foo() {
  let _: (Int) -> Void = Class.staticMethod
}

I have dumped an AST for this code and noticed that it tries to apply a property wrapper of the wrong type to an argument,
i.e. it tries to apply Int wrapper to an Int argument:

(argument_list implicit
  (argument
    (applied_property_wrapper_expr implicit type='Int'
      (declref_expr implicit type='Int' decl=main.(file).foo().autoclosure discriminator=0.autoclosure discriminator=1.value function_ref=unapplied))))

Such output produced only for non API-level property wrappers. It makes no sense for me, so I have considered it as a bug.

I have checked test files and noticed that they does not contain tests for non API-level property wrappers as parameters.
I have added more tests to cover such wrappers and encountered few other crashes.

I have changed few conditions that affect property wrapper application. Now argument declaration seems to be in order:

(argument_list implicit
  (argument
    (declref_expr implicit type='Int' decl=main.(file).foo().autoclosure discriminator=0.value function_ref=unapplied)))

Here are my changes to fix this crash.

[Sema] Added parameter tests for non-API-level wrappers.
[Sema] Added solution for non-API level property wrappers with closure.
@Vespen Vespen force-pushed the fix-non-api-wrapper-parameter branch from 9a2fdc2 to f1c6c1b Compare January 23, 2023 04:24
Copy link
Member

@hborla hborla left a comment

Choose a reason for hiding this comment

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

Thank you for the fix! I've left comments/questions inline.

@Vespen Vespen requested review from hborla and removed request for xedin and slavapestov January 25, 2023 06:36
@Vespen
Copy link
Contributor Author

Vespen commented Jan 25, 2023

Sorry, I have just requested review again and @xedin and @slavapestov were automatically removed. I am not able to add them again.

Copy link
Member

@hborla hborla left a comment

Choose a reason for hiding this comment

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

Thank you for the fix!

@hborla
Copy link
Member

hborla commented Jan 25, 2023

@swift-ci please smoke test

@xedin
Copy link
Contributor

xedin commented Jan 26, 2023

@swift-ci please test

@xedin xedin merged commit 516e616 into swiftlang:main Jan 26, 2023
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