Skip to content
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.

Sign up for GitHub

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

Jump to bottom

typeck: prohibit foreign statics w/ generics #65133

Merged
merged 1 commit into from
Oct 9, 2019
Merged

typeck: prohibit foreign statics w/ generics #65133

merged 1 commit into from
Oct 9, 2019

Conversation

davidtwco
Copy link
Member

@davidtwco davidtwco commented Oct 5, 2019 •
edited
Loading

Fixes #65035 and fixes #65025.

This PR modifies resolve to disallow foreign statics that have
generics.

improper_ctypes is not written to support type parameters, as these
are normally disallowed before the lint is run. Thus, type parameters in
foreign statics must be prohibited before the lint.

The only other case where this could have occured is in functions,
but typeck prohibits this with a "foreign items may not have type
parameters" error - a similar error did not exist for statics, because
statics cannot have type parameters, but they can use any
type parameters that are in scope (which isn't the case for functions).

@rust-highfive
Copy link
Collaborator

r? @cramertj

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 5, 2019
Centril
Centril reviewed Oct 6, 2019
View reviewed changes
src/librustc_typeck/check/mod.rs Outdated Show resolved Hide resolved
src/librustc_typeck/check/mod.rs Outdated Show resolved Hide resolved
@Centril
Copy link
Contributor

Centril commented Oct 6, 2019

statics cannot have type parameters, but they can use any
type parameters that are in scope (which isn't the case for functions).

Odd... ISTM that static should not be able to reference type parameter from an other scope either and that the failure should happen in resolve.

cc @petrochenkov

@petrochenkov
Copy link
Contributor

Yeah, this should be prevented by some barrier in resolve like ItemRibKind or something, at least for type parameters.

@davidtwco
Copy link
Member Author

Yeah, this should be prevented by some barrier in resolve like ItemRibKind or something, at least for type parameters.

I'll modify this PR to prohibit this in resolve instead.

@Centril
Copy link
Contributor

Centril commented Oct 6, 2019

r? @petrochenkov

@davidtwco davidtwco added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 6, 2019
@davidtwco davidtwco mentioned this pull request Oct 6, 2019
Closed
@davidtwco davidtwco force-pushed the issue-65035-static-with-generic-in-foreign-mod branch from bf1e55c to 1f7a051 Compare October 8, 2019 12:03
@davidtwco davidtwco added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 8, 2019
@davidtwco
Copy link
Member Author

davidtwco commented Oct 8, 2019 •
edited
Loading

@petrochenkov I've re-worked this PR to fix this in resolve instead.

I've introduced a StaticItemRibKind but that was only so I could avoid the misleading diagnostic, the actual prohibition of generics from foreign statics worked with just an ItemRibKind, so if you have a alternative approach to improving that diagnostic, I'm happy to do that.

(cc @varkor, this also resolves your comment about const params)

@petrochenkov
Copy link
Contributor

I've introduced a StaticItemRibKind but that was only so I could avoid the misleading diagnostic

I'd rather add a flag for this to ItemRibKind.

(FnItemRibKind is also very weird, all this stuff needs an audit (and the whole idea of conflating scopes with barriers is questionable), but that's for a different PR, I guess.)

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 8, 2019
@davidtwco davidtwco force-pushed the issue-65035-static-with-generic-in-foreign-mod branch from 1f7a051 to b514344 Compare October 8, 2019 16:33
@davidtwco
Copy link
Member Author

I'd rather add a flag for this to ItemRibKind.

@petrochenkov I've updated the PR to add a flag to ItemRibKind instead of StaticItemRibKind.

Centril
Centril reviewed Oct 8, 2019
View reviewed changes
src/librustc_resolve/late.rs Show resolved Hide resolved
src/librustc_resolve/late.rs Outdated Show resolved Hide resolved
src/librustc_resolve/late.rs Outdated Show resolved Hide resolved
@davidtwco davidtwco force-pushed the issue-65035-static-with-generic-in-foreign-mod branch from b514344 to 5a46a09 Compare October 8, 2019 17:32
@davidtwco
resolve: prohibit foreign statics w/ generics
ccbf2b7 
This commit modifies resolve to disallow foreign statics that use
parent generics.

`improper_ctypes` is not written to support type parameters, as these
are normally disallowed before the lint is run. Thus, type parameters in
foreign statics must be prohibited before the lint.

The only other case where this *could* have occured is in functions,
but typeck prohibits this with a "foreign items may not have type
parameters" error - a similar error did not exist for statics, because
statics cannot have type parameters, but they can use any
type parameters that are in scope (which isn't the case for functions).

Signed-off-by: David Wood <david@davidtw.co>
@davidtwco davidtwco force-pushed the issue-65035-static-with-generic-in-foreign-mod branch from 5a46a09 to ccbf2b7 Compare October 8, 2019 17:51
@petrochenkov
Copy link
Contributor

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Oct 8, 2019

📌 Commit ccbf2b7 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 8, 2019
Centril added a commit to Centril/rust that referenced this pull request Oct 8, 2019
@Centril
Rollup merge of rust-lang#65133 - davidtwco:issue-65035-static-with-g…
9b989ea 
…eneric-in-foreign-mod, r=petrochenkov

typeck: prohibit foreign statics w/ generics

Fixes rust-lang#65035 and fixes rust-lang#65025.

This PR modifies resolve to disallow foreign statics that have
generics.

`improper_ctypes` is not written to support type parameters, as these
are normally disallowed before the lint is run. Thus, type parameters in
foreign statics must be prohibited before the lint.

The only other case where this *could* have occured is in functions,
but typeck prohibits this with a "foreign items may not have type
parameters" error - a similar error did not exist for statics, because
statics cannot have type parameters, but they can use any
type parameters that are in scope (which isn't the case for functions).
@Centril Centril mentioned this pull request Oct 8, 2019
Merged
bors added a commit that referenced this pull request Oct 8, 2019
@bors
Auto merge of #65223 - Centril:rollup-5sdvdni, r=Centril
b5bd31e 
Rollup of 7 pull requests

Successful merges:

 - #64284 (Warn if include macro fails to include entire file)
 - #65081 (Remove -Zprofile-queries)
 - #65133 (typeck: prohibit foreign statics w/ generics)
 - #65135 (Add check for missing tests for error codes)
 - #65141 (Replace code of conduct with link)
 - #65194 (Use structured suggestion for removal of `as_str()` call)
 - #65213 (Ignore `ExprKind::DropTemps` for some ref suggestions)

Failed merges:

r? @ghost
@bors bors merged commit ccbf2b7 into rust-lang:master Oct 9, 2019
@davidtwco davidtwco deleted the issue-65035-static-with-generic-in-foreign-mod branch October 11, 2019 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Centril Centril Centril left review comments

@varkor varkor varkor left review comments

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@petrochenkov petrochenkov

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
7 participants
@davidtwco @rust-highfive @Centril @petrochenkov @bors @varkor @cramertj