[SR-12033] [Sema] Do not allow inferring defaultable closure `() -> $T` for autoclosure arguments result #35503

LucianoPAlmeida · 2021-01-20T01:25:17Z

Prevent defaultable closure binding to be added if its connected to an auto closure argument.
Sorry it took some time, but have been between other things and actually giving a thought if this would be the correct
approach and about source compatibility in this case.

Resolves SR-12033.

lib/Sema/CSSimplify.cpp

xedin

Looks much better, just need to figure out how to avoid adding new flags to the binding set (since they can't be maintained in incremental mode I'm working on).

LucianoPAlmeida · 2021-01-24T02:04:31Z

Looks much better, just need to figure out how to avoid adding new flags to the binding set (since they can't be maintained in incremental mode I'm working on).

Great, I made some changes and now to track the exact DefaultConstraints that are "disallowed" instead of using a flag.
I've been following the recently PRs related to bindings(normally trying to learn something) but the understanding I have for what "incremental mode" is very little. So let me know if that should be sufficient or if I should try another approach =]

xedin · 2021-01-24T04:44:11Z

I'm working on making it right now. The idea is that determineBestBindings can avoid having to gather constraints and re-compute bindings all the time since that's just wasted work if constraints didn't change, instead bindings could be tracked in constraint graph as constraints are introduced and removed.

LucianoPAlmeida · 2021-01-25T02:50:33Z

I'm working on making it right now. The idea is that determineBestBindings can avoid having to gather constraints and re-compute bindings all the time since that's just wasted work if constraints didn't change, instead bindings could be tracked in constraint graph as constraints are introduced and removed.

Awesome, it seems that it will give a good speed up on inference 🎉
So for this PR, should we wait on those changes to land to avoid any issues?

xedin · 2021-01-25T17:47:11Z

No really, it shouldn't collide with that work if everything is done in a stateless manner.

LucianoPAlmeida · 2021-01-26T03:18:25Z

No really, it shouldn't collide with that work if everything is done in a stateless manner.

Right! I modify the approach to a way where we don't have a state on the binding set.
So let me know what you think :)

xedin

This is still state although passed in. Need to figure out how to do that without any form of state. Alternative solution might be to add that constraint to the binding set (since it doesn't affect the score) and remove it during finalization.

LucianoPAlmeida · 2021-01-26T23:35:51Z

This is still state although passed in. Need to figure out how to do that without any form of state. Alternative solution might be to add that constraint to the binding set (since it doesn't affect the score) and remove it during finalization.

Ah ok, I see thanks :)
My understanding of state was never left anything on the binding set object. That is why I thought this will be fine to hold just while running the set of constraints, sorry for that.

xedin · 2021-01-26T23:36:49Z

No worries, I should have been more precise :)

LucianoPAlmeida · 2021-01-27T02:51:18Z

Alternative solution might be to add that constraint to the binding set (since it doesn't affect the score) and remove it during finalization.

Right, looking into it!

LucianoPAlmeida · 2021-01-29T03:19:59Z

Alternative solution might be to add that constraint to the binding set (since it doesn't affect the score) and remove it during finalization.

Hey @xedin :)
When you say "removing during finalization", do you mean remove the defaults from the set on finalize? Or actually removing the Bindings from default that are expected to be on the set at this point?

I'm trying to figure out why given:

$T3 arg conv $T2 [apply argument -> comparing call argument #0 to parameter #0 -> @autoclosure result];
$T3 closure can default to () -> $T4

If I try wait for finalization $T2 still gets bound to defaulted ($T2 potentially_incomplete involves_type_vars bindings={} defaults={() -> $T4}), even removing defaults for $T3 adjacents as well from inferredBindings (which is just $T2) .

xedin · 2021-01-29T19:55:09Z

Yes, my suggesting what dealing with it during finalize. The behavior you see is related to transitive inference, $T2 has access to bindings of $T3 and it know that it could be bound to any supertype of $T3 due to arg conversion constraint so it infers defaults from $T3.

LucianoPAlmeida · 2021-01-29T21:05:05Z

The behavior you see is related to transitive inference, $T2 has access to bindings of $T3 and it know that it could be bound to any supertype of $T3 due to arg conversion constraint so it infers defaults from $T3.

Ah that makes sense, thanks :)

lib/Sema/CSBindings.cpp

…sures arguments

LucianoPAlmeida · 2021-02-05T23:41:10Z

@swift-ci Please smoke test

LucianoPAlmeida · 2021-02-05T23:41:17Z

@swift-ci Please test source compatibility

LucianoPAlmeida · 2021-02-06T10:15:12Z

@swift-ci Please test source compatibility

LucianoPAlmeida · 2021-02-06T10:15:36Z

@swift-ci Please smoke test MacOS platform

LucianoPAlmeida · 2021-02-06T13:21:56Z

Compat suit seems to be broken on building spm

09:41:37 fatal error
09:41:37 : module map file '/Users/buildnode/jenkins/workspace-private/swift-PR-source-compat-suite/build/compat_macos/swiftpm-macosx-x86_64/apple/Intermediates.noindex/GeneratedModuleMaps/macosx/CCryptoBoringSSL.modulemap' not found

LucianoPAlmeida · 2021-02-06T13:27:24Z

I'm still looking but suspect it could be something related to recent changes on apple/swift-crypto#71

LucianoPAlmeida · 2021-02-06T13:33:55Z

swiftlang/swift-package-manager#3238
@swift-ci Please test source compatibility

LucianoPAlmeida · 2021-02-06T16:05:45Z

Ping @compnerd @yim-lee
To check if compat issue is indeed related to that PR? :)

yim-lee · 2021-02-06T17:00:45Z

We haven't merged swiftlang/swift-package-manager#3238 yet but could be related to swiftlang/swift-package-manager#3202. Investigating.

compnerd · 2021-02-06T17:32:24Z

I think that @yim-lee is correct about swiftlang/swift-package-manager#3202 raising this issue - but I'm not sure I understand how it is causing the issue as of yet. It seems to be looking for a generated modulemap, which is not needed as swift-crypto provides a pre-generated one. I wonder if there is some cached content.

LucianoPAlmeida · 2021-02-06T17:51:29Z

Thanks for looking @yim-lee @compnerd =]

Seems like the clean suit didn't trigger

yim-lee · 2021-02-06T18:03:34Z

@LucianoPAlmeida We will need @shahmishal's help with #35503 (comment). Sorry for the trouble!

LucianoPAlmeida · 2021-02-06T18:17:42Z

@LucianoPAlmeida We will need @shahmishal's help with #35503 (comment). Sorry for the trouble!

No worries! Thank you for looking :))

LucianoPAlmeida · 2021-02-09T10:41:19Z

@swift-ci Please test source compatibility

LucianoPAlmeida · 2021-02-09T22:11:04Z

Seems like there is a FAIL is source compatibility suit https://ci.swift.org/job/swift-PR-source-compat-suite-debug/3508//artifact/swift-source-compat-suite/FAIL_Dollar-Dollar.xcodeproj_4.0_BuildXcodeProjectScheme_Dollar_generic-platform-macOS.log, I'm still not sure how those changes could cause this issue... it seems unrelated but I can't say for sure ...

shahmishal · 2021-02-09T22:23:17Z

This is the same failure on the main branch, it's not related to your change.

LucianoPAlmeida · 2021-02-10T00:04:57Z

This is the same failure on the main branch, it's not related to your change.

Ah great, thank you @shahmishal :)

LucianoPAlmeida · 2021-02-10T00:23:33Z

So @xedin anything else on this one?

xedin · 2021-02-10T00:25:15Z

Nope, this is great :)

LucianoPAlmeida · 2021-02-10T00:25:43Z

Haha I ping you at the same time 🤣 😆
Thank you! :)

LucianoPAlmeida requested a review from xedin January 20, 2021 01:25

xedin reviewed Jan 20, 2021

View reviewed changes

lib/Sema/CSSimplify.cpp Outdated Show resolved Hide resolved

LucianoPAlmeida force-pushed the SR-12033-autoclosure branch from ea8108a to 3de257e Compare January 23, 2021 04:54

LucianoPAlmeida requested a review from xedin January 23, 2021 04:56

xedin reviewed Jan 23, 2021

View reviewed changes

LucianoPAlmeida force-pushed the SR-12033-autoclosure branch from 3de257e to e174c5f Compare January 24, 2021 01:55

LucianoPAlmeida force-pushed the SR-12033-autoclosure branch from e174c5f to e28a822 Compare January 26, 2021 03:14

xedin reviewed Jan 26, 2021

View reviewed changes

LucianoPAlmeida force-pushed the SR-12033-autoclosure branch from e28a822 to 0b75ade Compare January 29, 2021 03:20

LucianoPAlmeida force-pushed the SR-12033-autoclosure branch from 0b75ade to ae0bf9f Compare January 30, 2021 18:09

LucianoPAlmeida requested a review from xedin January 30, 2021 18:10

LucianoPAlmeida force-pushed the SR-12033-autoclosure branch 2 times, most recently from 6a1a511 to 053db31 Compare January 31, 2021 23:04

xedin reviewed Feb 1, 2021

View reviewed changes

lib/Sema/CSBindings.cpp Outdated Show resolved Hide resolved

LucianoPAlmeida force-pushed the SR-12033-autoclosure branch 3 times, most recently from 6b89be0 to b5b687a Compare February 5, 2021 03:10

LucianoPAlmeida requested a review from xedin February 5, 2021 03:14

xedin reviewed Feb 5, 2021

View reviewed changes

lib/Sema/CSBindings.cpp Show resolved Hide resolved

LucianoPAlmeida added 2 commits February 5, 2021 20:20

[CSBindings] Prevent solver from infering default binding to auto clo…

fccb3e0

…sures arguments

[tests] Adding SR-12033 regression test cases

3330083

LucianoPAlmeida force-pushed the SR-12033-autoclosure branch from e4e78c2 to 3330083 Compare February 5, 2021 23:21

LucianoPAlmeida mentioned this pull request Feb 6, 2021

[ownership] Move lowering past SILCombine and right before the inliner. #35802

Merged

xedin approved these changes Feb 10, 2021

View reviewed changes

xedin merged commit daa4fff into swiftlang:main Feb 10, 2021

LucianoPAlmeida deleted the SR-12033-autoclosure branch February 10, 2021 00:25

xedin mentioned this pull request Feb 17, 2021

Revert "[SR-12033] [Sema] Do not allow inferring defaultable closure () -> $T for autoclosure arguments result" #36022

Merged

LucianoPAlmeida mentioned this pull request Feb 19, 2021

[CSBindings] Avoid to infer transitive defaults for DefaultClosureType #36043

Merged

LucianoPAlmeida mentioned this pull request Feb 11, 2021

[SR-12033] Error or inconsistency in capturing variable by @autoclosure closure #54470

Closed

[SR-12033] [Sema] Do not allow inferring defaultable closure () -> $T for autoclosure arguments result #35503

[SR-12033] [Sema] Do not allow inferring defaultable closure () -> $T for autoclosure arguments result #35503

Uh oh!

Conversation

LucianoPAlmeida commented Jan 20, 2021

Uh oh!

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

LucianoPAlmeida commented Jan 24, 2021

Uh oh!

xedin commented Jan 24, 2021

Uh oh!

LucianoPAlmeida commented Jan 25, 2021

Uh oh!

xedin commented Jan 25, 2021

Uh oh!

LucianoPAlmeida commented Jan 26, 2021

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

LucianoPAlmeida commented Jan 26, 2021

Uh oh!

xedin commented Jan 26, 2021

Uh oh!

LucianoPAlmeida commented Jan 27, 2021

Uh oh!

LucianoPAlmeida commented Jan 29, 2021

Uh oh!

xedin commented Jan 29, 2021

Uh oh!

LucianoPAlmeida commented Jan 29, 2021

Uh oh!

Uh oh!

Uh oh!

LucianoPAlmeida commented Feb 5, 2021

Uh oh!

LucianoPAlmeida commented Feb 5, 2021

Uh oh!

LucianoPAlmeida commented Feb 6, 2021

Uh oh!

LucianoPAlmeida commented Feb 6, 2021

Uh oh!

LucianoPAlmeida commented Feb 6, 2021

Uh oh!

LucianoPAlmeida commented Feb 6, 2021

Uh oh!

LucianoPAlmeida commented Feb 6, 2021

Uh oh!

LucianoPAlmeida commented Feb 6, 2021

Uh oh!

yim-lee commented Feb 6, 2021

Uh oh!

compnerd commented Feb 6, 2021

Uh oh!

LucianoPAlmeida commented Feb 6, 2021

Uh oh!

yim-lee commented Feb 6, 2021

Uh oh!

LucianoPAlmeida commented Feb 6, 2021

Uh oh!

LucianoPAlmeida commented Feb 9, 2021

Uh oh!

LucianoPAlmeida commented Feb 9, 2021

Uh oh!

shahmishal commented Feb 9, 2021

Uh oh!

LucianoPAlmeida commented Feb 10, 2021

Uh oh!

LucianoPAlmeida commented Feb 10, 2021

Uh oh!

xedin commented Feb 10, 2021

Uh oh!

LucianoPAlmeida commented Feb 10, 2021

Uh oh!

Uh oh!

[SR-12033] [Sema] Do not allow inferring defaultable closure `() -> $T` for autoclosure arguments result #35503

[SR-12033] [Sema] Do not allow inferring defaultable closure `() -> $T` for autoclosure arguments result #35503