Skip to content

Swift 4 compatibility hack for redeclaration of properties in generic type and extension #17392

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 2 commits into from
Jun 22, 2018

Conversation

huonw
Copy link
Contributor

@huonw huonw commented Jun 21, 2018

The patch that nailed down our semantics here missed an additional case that
required a compatibility hack: a property on a generic type and a same-named one
in an (unconstrained) extension:

struct Foo<T> {
    var x: Int { return 0 }
}
extension Foo {
    var x: Bool { return false }
}

Fixes rdar://problem/40685642.

"illegal" is an overloaded word and can distress people who aren't familiar with
the usage as being illegal with respect to the language definition, not the
actual law.
@huonw huonw requested a review from jrose-apple June 21, 2018 07:41
@huonw
Copy link
Contributor Author

huonw commented Jun 21, 2018

@swift-ci please smoke test

@huonw huonw force-pushed the redefinition-swift-4-compat branch from e5ef10d to 76924b9 Compare June 21, 2018 12:26
@huonw
Copy link
Contributor Author

huonw commented Jun 21, 2018

@swift-ci please smoke test

Copy link
Contributor

@jrose-apple jrose-apple left a comment

Choose a reason for hiding this comment

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

Seems reasonable, if unfortunate.

// matching the Swift 4 behaviour (i.e. just emitting the future-compat
// warning) will result in SILGen crashes due to both properties mangling
// the same, so it's better to just follow the Swift 5 behaviour and emit
// the actual error.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a test case for this?

Copy link
Contributor Author

@huonw huonw Jun 21, 2018

Choose a reason for hiding this comment

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

The z property in the diff for the overload.swift is this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, right, thanks.

lib/AST/Decl.cpp Outdated
@@ -1747,7 +1747,31 @@ bool swift::conflicting(ASTContext &ctx,
}

// Otherwise, the declarations conflict if the overload types are the same.
return sig1Type == sig2Type;
if (sig1Type == sig2Type) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: early return.

…tensions of generic types.

The patch that nailed down our semantics here missed an additional case that
required a compatibility hack: a property on a generic type and a same-named one
in an (unconstrained) extension:

    struct Foo<T> {
        var x: Int { return 0 }
    }
    extension Foo {
        var x: Bool { return false }
    }

Fixes rdar://problem/40685642.
@huonw huonw force-pushed the redefinition-swift-4-compat branch from 76924b9 to b96aeda Compare June 21, 2018 22:43
@huonw
Copy link
Contributor Author

huonw commented Jun 21, 2018

@swift-ci please smoke test

@huonw huonw merged commit 73d09e5 into swiftlang:master Jun 22, 2018
@huonw huonw deleted the redefinition-swift-4-compat branch June 22, 2018 01:23
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.

2 participants