-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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
[Sema] Diagnose internal(set) from @inlinable functions #21033
[Sema] Diagnose internal(set) from @inlinable functions #21033
Conversation
9ac1139
to
0a0bd7b
Compare
0a0bd7b
to
aa8b5e4
Compare
@swift-ci please test |
Build failed |
Build failed |
|
||
NOTE(resilience_decl_declared_here, | ||
none, "%0 %1 is not '@usableFromInline' or public", (DescriptiveDeclKind, DeclName)) | ||
none, DECL_OR_ACCESSOR "2 %1 is not '@usableFromInline' or public", | ||
(DescriptiveDeclKind, DeclName, bool)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make sure each of these diagnostics is tested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There currently aren't any tests for the downgradeToWarning
variant of this. Should I copy test/attr/attr_inlinable.swift
and rewrite the expected-error
s?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes please! cc @slavapestov just to make sure they're not hidden somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do this in a followup PR now Gonna roll this in because there are other changes I'm making.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take a look at test/Compatibility/attr_inlinable*.swift
Note that if we can't take this on the Swift 5 branch it all gets trickier w/r/t source compat / when to use warnings vs. errors… |
3c5eba9
to
42233e4
Compare
@swift-ci please test |
Build failed |
Build failed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good except for one nit
public internal(set) var x: Int // expected-note {{setter for 'x' is not '@usableFromInline' or public}} | ||
|
||
@inlinable public mutating func setsX() { | ||
x = 10 // expected-error {{setter for 'x' is internal and cannot be referenced from an '@inlinable' function}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, wait, we can't make this an error in 4.2 mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the PR for this, but should I update attr_inlinable.swift
to Swift 5 and put the 4.2 compat stuff in Compatibility/attr_inlinable_swift42.swift
? Or leave it where there's a Compatibility/attr_inlinable_swift5.swift
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@slavapestov Any preference? I think we've been leaning towards always putting the latest behavior in the "normal" file and older behavior in "Compatibility".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went ahead and just did that
42233e4
to
0701f81
Compare
Also add a compatibility test for Swift 4.2 behavior
Previously, members of protocols that were not protocol requirements, like accessors and typealiases, did not inherit @usableFromInline from the parent protocol. Change this so they do.
This patch mainly consolidates the functions used to check accessors vs. other decls, and makes sure we check setter access as well as regular decl access. rdar://45217648
0701f81
to
1abeeb8
Compare
@swift-ci please test |
Build failed |
Build failed |
This patch mainly consolidates the functions used to check accessors vs.
other decls, and makes sure we check setter access as well as regular
decl access.
rdar://45217648
Resolves SR-8969