Skip to content

[CS] Add special case to preserve compatibility for rdar://84279742 #39852

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 1 commit into from
Oct 21, 2021

Conversation

hamishknight
Copy link
Contributor

@hamishknight hamishknight commented Oct 21, 2021

When ranking constructor parameter lists, we compose them as tuples or parens, and check if they are subtypes or unlabeled versions of each other. Previously this was done with the parameter flags intact, but recently in #39543 I changed the logic to explicitly strip parameter flags in preparation for no longer storing the flags on these types.

This caused a slight behavior change, as it turns out we have a special case in TupleType::get that allows an unlabeled single parameter to be composed as a tuple type if its variadic bit is set. With the parameter flags now stripped, we produce a paren type. This means that when comparing the parameter lists e.g (x: Int...) and (Int...), instead of comparing two tuple types, we end up comparing a tuple with a paren and fail.

To preserve the old behavior, implement a special case for when we have an unlabeled and labeled variadic comparison for a single parameter. In this case, add the parameter types directly to the type diff, and track which one had the label. The ranking logic can then use this to prefer the unlabeled variant. This is only needed in the single parameter case, as other cases will compare as tuples the same as before. In cases where variadics aren't used, we may end up trying to compare parens with tuples, but that's consistent with what we previously did.

SR-15362
rdar://84279742

When ranking constructor parameter lists, we
compose them as tuples or parens, and check if
they are subtypes or unlabeled versions of each
other. Previously this was done with the parameter
flags intact, but recently I changed the logic to
explicitly strip parameter flags in preparation
for no longer storing the flags on these types.

This caused a slight behavior change, as it turns
out we have a special case in `TupleType::get`
that allows an unlabeled single parameter to be
composed as a tuple type if its variadic bit is
set. With the parameter flags now stripped, we
produce a paren type. This means that when
comparing the parameter lists e.g `(x: Int...)`
and `(Int...)`, instead of comparing two tuple
types end up comparing a tuple with a paren and
fail.

To preserve the old behavior, implement a special
case for when we have an unlabeled and labeled
variadic comparison for a single parameter. In
this case, add the parameter types directly to the
type diff, and track which one had the label. The
ranking logic can then use this to prefer the
unlabeled variant. This is only needed in the
single parameter case, as other cases will compare
as tuples the same as before. In cases where
variadics aren't used, we may end up trying to
compare parens with tuples, but that's consistent
with what we previously did.

rdar://84279742
@hamishknight
Copy link
Contributor Author

@swift-ci please test

@hamishknight
Copy link
Contributor Author

@swift-ci please test source compatibility

@hamishknight hamishknight requested a review from xedin October 21, 2021 12:59
@Kyle-Ye
Copy link
Contributor

Kyle-Ye commented Oct 21, 2021

Just out of curious, I'd like to know why the change only effect Linux not macOS? It seems the lib/Sema/CSRanking.cpp is used both on Linux and macOS. @hamishknight

@hamishknight
Copy link
Contributor Author

@Kyle-Ye Hmm rdar://84279742 says it also failed for macOS. I'm not aware of any compiler behavior that would cause the ranking to differ between macOS and Linux, are there any #if blocks in the project that might change the overload set for the initializer call depending on the platform?

@Kyle-Ye
Copy link
Contributor

Kyle-Ye commented Oct 21, 2021

@Kyle-Ye Hmm rdar://84279742 says it also failed for macOS. I'm not aware of any compiler behavior that would cause the ranking to differ between macOS and Linux, are there any #if blocks in the project that might change the overload set for the initializer call depending on the platform?

Because my PR and other's PR are both failed on the Linux Platform not on the macOS Platform.
Like this one swiftlang/swift-markdown#8

I see the Linux Test is using the ubuntu1602-2021-10-17 snapshot of Swift but did not see which version the macOS Platform is using on the env.

@hamishknight
Copy link
Contributor Author

@Kyle-Ye Ah it looks like the Linux run is building off the latest main snapshot, however the macOS run appears to be using the CI toolchain, which will be an older version of Swift. That would explain the difference in behavior.

FWIW I can confirm that using my local build of Swift on macOS, swift-markdown fails to build without these changes, and successfully builds with them.

@hamishknight
Copy link
Contributor Author

@swift-ci please smoke test Linux

@swiftlang swiftlang deleted a comment from swift-ci Oct 21, 2021
@hamishknight hamishknight merged commit 334bde6 into swiftlang:main Oct 21, 2021
@hamishknight hamishknight deleted the enigma-variadics branch October 21, 2021 22:13
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