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

Do not allow extern unsized_fn_param #123894

Closed
wants to merge 3 commits into from
Closed

Do not allow extern unsized_fn_param #123894

wants to merge 3 commits into from

Conversation

eggyal
Copy link
Contributor

@eggyal eggyal commented Apr 13, 2024

Fixes #123887

@rustbot
Copy link
Collaborator

rustbot commented Apr 13, 2024

r? @estebank

rustbot has assigned @estebank.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 13, 2024
compiler-errors
compiler-errors reviewed Apr 13, 2024
View reviewed changes
tests/ui/unsized-locals/extern-parameter-issue-123887.rs
type ExternType;
}

fn f(_: ExternType) {} //~ ERROR the size for values of type `ExternType` cannot be known at compilation time
Copy link
Member

Choose a reason for hiding this comment

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

Also, what happens when we monomorphize some fn f<T: ?Sized>(_: T) by calling it with a value of ExternType?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another good point! I'm not hugely familiar with the monomorphization code, but have pushed a solution d98f42b. I'm not hugely comfortable with it though:

  1. it can trigger new recursion depth failures (perhaps this can't be avoided however?);
  2. I guess it could end up being evaluated for the same mono item multiple times, which would be unnecessary—but I don't quite understand the monomorphization process well enough to see how this can best be avoided?
  3. does it need a perf run?

@compiler-errors
Copy link
Member

somewhat afraid this solution is not general enough -- can you check that this makes sense in other related cases?

r? compiler-errors
@rustbot author

@rustbot rustbot assigned compiler-errors and unassigned estebank Apr 13, 2024
@rustbot rustbot 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 Apr 13, 2024
@compiler-errors
Copy link
Member

compiler-errors commented Apr 14, 2024 •
edited
Loading

Hm -- I believe might be the correct approach if we were going to stabilize unsized_fn_params and extern_types in the state that they exist right now. However, extern_types still has open questions around how to deal with their sizes and metadata in general, and unsized_fn_params is very likely never going to get stabilized.

The problem here is that if we were to check unsized param validity in a principled way, it would be making sure that all params are T: ?Sized + DynamicallySized (or some other trait that says that they are unsized but we can compute their size for the purpose of alloca). Then we wouldn't need to re-check all of this during monomorphization, which any approach like this would need to do and which feels unfortunate :/

Honestly unsized_fn_params should probably just be marked internal and this test could be committed as a known-bug, but that's really it. I'm gonna close this because I don't believe these checks are really worth the extra complication in the compiler. Thanks for the PR though!

@eggyal eggyal deleted the issue-123887 branch April 14, 2024 14:52
@eggyal eggyal mentioned this pull request Apr 14, 2024
Closed
@RalfJung RalfJung mentioned this pull request Jun 22, 2024
Merged
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jun 23, 2024
@compiler-errors
Rollup merge of rust-lang#126830 - RalfJung:unsized-fn-params, r=comp…
7c3c544 
…iler-errors

make unsized_fn_params an internal feature

As suggested [here](rust-lang#123894 (comment)).
r? `@compiler-errors`

Fixes rust-lang#123887 (kind of -- ICEs on internal features are considered acceptable so this issue is not-a-bug once this PR lands)
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jun 23, 2024
@compiler-errors
Rollup merge of rust-lang#126830 - RalfJung:unsized-fn-params, r=comp…
54cf62e 
…iler-errors

make unsized_fn_params an internal feature

As suggested [here](rust-lang#123894 (comment)).
r? ``@compiler-errors``

Fixes rust-lang#123887 (kind of -- ICEs on internal features are considered acceptable so this issue is not-a-bug once this PR lands)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 23, 2024
@matthiaskrgr
Rollup merge of rust-lang#126830 - RalfJung:unsized-fn-params, r=comp…
0a7adaf 
…iler-errors

make unsized_fn_params an internal feature

As suggested [here](rust-lang#123894 (comment)).
r? `@compiler-errors`

Fixes rust-lang#123887 (kind of -- ICEs on internal features are considered acceptable so this issue is not-a-bug once this PR lands)
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 23, 2024
@rust-timer
Unrolled build for rust-lang#126830
e8be16e 
Rollup merge of rust-lang#126830 - RalfJung:unsized-fn-params, r=compiler-errors

make unsized_fn_params an internal feature

As suggested [here](rust-lang#123894 (comment)).
r? `@compiler-errors`

Fixes rust-lang#123887 (kind of -- ICEs on internal features are considered acceptable so this issue is not-a-bug once this PR lands)
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Jun 24, 2024
@matthiaskrgr
Rollup merge of #126830 - RalfJung:unsized-fn-params, r=compiler-errors
4a51364 
make unsized_fn_params an internal feature

As suggested [here](rust-lang/rust#123894 (comment)).
r? `@compiler-errors`

Fixes rust-lang/rust#123887 (kind of -- ICEs on internal features are considered acceptable so this issue is not-a-bug once this PR lands)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compiler-errors compiler-errors compiler-errors left review comments

Assignees

@compiler-errors compiler-errors

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ICE: unsized arguments must not be 'extern' types
4 participants
@eggyal @rustbot @compiler-errors @estebank