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.

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

Improve detection of generics on lang items #87875

Merged
merged 1 commit into from
Aug 25, 2021

Conversation

asquared31415
Copy link
Contributor

Adds detection for the required generics for all lang items. Many lang items require an exact or minimum amount of generic arguments and if they don't exist, the compiler will ICE. This does not add any additional validation about bounds on generics or any other lang item restrictions.

Fixes one of the ICEs in #87573

cc @FabianWolff

@rust-highfive
Copy link
Collaborator

r? @LeSeulArtichaut

(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 Aug 9, 2021
@asquared31415
Copy link
Contributor Author

Somewhat related to #9307, but not entirely, and does not close it

@rust-log-analyzer

This comment has been minimized.

@asquared31415
Copy link
Contributor Author

Whelp I "cleaned up" the code and didn't rerun tests by mistake, used a != to compare "less than" by mistake

@rust-log-analyzer

This comment has been minimized.

@asquared31415
Copy link
Contributor Author

It compiles locally with that feature enabled, weird, guess it's stabilized now though

Copy link
Contributor

@LeSeulArtichaut LeSeulArtichaut left a comment

Choose a reason for hiding this comment

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

I'm not familiar at all with this code, so I'm going to pass this to someone else.

compiler/rustc_hir/src/lang_items.rs Outdated Show resolved Hide resolved
@LeSeulArtichaut
Copy link
Contributor

Per the GitHub suggestions: r? @cjgillot do you want to take this?

Copy link
Contributor

@cjgillot cjgillot left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @asquared31415. I left a few nits.
Could you also squash the commits?

compiler/rustc_passes/src/lang_items.rs Outdated Show resolved Hide resolved
// traits have one for the argument list, generators have one for the
// resume argument, and ordering/equality relations have one for the RHS
// Some other types like Box and various functions like drop_in_place
// have minimum requirements.
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC, minimum requirements exist in cases where the other generics can be inferred, either through a function parameter or through defaulted parameters. So, why do some traits (Unsize, CoerceUnsized) have minimum requirements?

@@ -0,0 +1,14 @@
#![feature(lang_items,no_core)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can all those tests be put in a single file?

Copy link
Contributor Author

@asquared31415 asquared31415 Aug 15, 2021

Choose a reason for hiding this comment

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

I had to make it two, because I have two tests that should compile for a couple different lang items, and one for the ones that error. But they have been merged into two files in the lang-items directory root.

@cjgillot cjgillot 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 Aug 14, 2021
@asquared31415
Copy link
Contributor Author

Oh no CI failed enough that the bot didn't even post a comment about the failure. How did this break this badly.

@asquared31415
Copy link
Contributor Author

I'm not sure how to resolve that CI issue. It looks like it's identical to src/test/ui/error-codes/E0152.rs before my changes, but I don't know where that test that is failing CI comes from.

@asquared31415
Copy link
Contributor Author

Nevermind, I found it, it was in the E0152.md file in rustc_error_codes

@cjgillot
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Aug 23, 2021

📌 Commit 385a233 has been approved by cjgillot

@bors bors removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 23, 2021
@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 24, 2021
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@inquisitivecrystal inquisitivecrystal added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 24, 2021
@asquared31415
Copy link
Contributor Author

I'm fairly certain that failure was spurious, is there any action I need to take to get it to retry?

@BoxyUwU
Copy link
Member

BoxyUwU commented Aug 25, 2021

@bors retry

@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-review Status: Awaiting review from the assignee but also interested parties. labels Aug 25, 2021
@bors
Copy link
Contributor

bors commented Aug 25, 2021

⌛ Testing commit 385a233 with merge 9863bf5...

@bors
Copy link
Contributor

bors commented Aug 25, 2021

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 9863bf5 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 25, 2021
@bors bors merged commit 9863bf5 into rust-lang:master Aug 25, 2021
@rustbot rustbot added this to the 1.56.0 milestone Aug 25, 2021
@asquared31415 asquared31415 deleted the generic-lang-items branch August 29, 2021 22:54
Manishearth added a commit to Manishearth/rust that referenced this pull request Sep 14, 2021
Fix ICE when `start` lang item has wrong generics

In my previous pr rust-lang#87875 I missed the requirements on the `start` lang item due to its relative difficulty to test and opting for more conservative estimates.  This fixes that by updating the requirement to be exactly one generic type.

The `start` lang item should have exactly one generic type for the return type of the `main` fn ptr passed to it.  I believe having zero would previously *sometimes* compile (often with the use of `fn() -> ()` as the fn ptr but it was likely UB to call if the return type of `main` was not `()` as far as I know) however it also sometimes would not for various errors including ICEs and LLVM errors depending on exact situations.  Having more than 1 generic has always failed with an ICE because only the one generic type is expected and provided.

Fixes rust-lang#79559, fixes rust-lang#73584, fixes rust-lang#83117 (all duplicates)
Relevant to rust-lang#9307

r? ``@cjgillot``
ehuss added a commit to ehuss/rust that referenced this pull request Sep 30, 2021
Fix ICE when `start` lang item has wrong generics

In my previous pr rust-lang#87875 I missed the requirements on the `start` lang item due to its relative difficulty to test and opting for more conservative estimates.  This fixes that by updating the requirement to be exactly one generic type.

The `start` lang item should have exactly one generic type for the return type of the `main` fn ptr passed to it.  I believe having zero would previously *sometimes* compile (often with the use of `fn() -> ()` as the fn ptr but it was likely UB to call if the return type of `main` was not `()` as far as I know) however it also sometimes would not for various errors including ICEs and LLVM errors depending on exact situations.  Having more than 1 generic has always failed with an ICE because only the one generic type is expected and provided.

Fixes rust-lang#79559, fixes rust-lang#73584, fixes rust-lang#83117 (all duplicates)
Relevant to rust-lang#9307

r? `@cjgillot`
Manishearth added a commit to Manishearth/rust that referenced this pull request Sep 30, 2021
Fix ICE when `start` lang item has wrong generics

In my previous pr rust-lang#87875 I missed the requirements on the `start` lang item due to its relative difficulty to test and opting for more conservative estimates.  This fixes that by updating the requirement to be exactly one generic type.

The `start` lang item should have exactly one generic type for the return type of the `main` fn ptr passed to it.  I believe having zero would previously *sometimes* compile (often with the use of `fn() -> ()` as the fn ptr but it was likely UB to call if the return type of `main` was not `()` as far as I know) however it also sometimes would not for various errors including ICEs and LLVM errors depending on exact situations.  Having more than 1 generic has always failed with an ICE because only the one generic type is expected and provided.

Fixes rust-lang#79559, fixes rust-lang#73584, fixes rust-lang#83117 (all duplicates)
Relevant to rust-lang#9307

r? ``@cjgillot``
Manishearth added a commit to Manishearth/rust that referenced this pull request Sep 30, 2021
Fix ICE when `start` lang item has wrong generics

In my previous pr rust-lang#87875 I missed the requirements on the `start` lang item due to its relative difficulty to test and opting for more conservative estimates.  This fixes that by updating the requirement to be exactly one generic type.

The `start` lang item should have exactly one generic type for the return type of the `main` fn ptr passed to it.  I believe having zero would previously *sometimes* compile (often with the use of `fn() -> ()` as the fn ptr but it was likely UB to call if the return type of `main` was not `()` as far as I know) however it also sometimes would not for various errors including ICEs and LLVM errors depending on exact situations.  Having more than 1 generic has always failed with an ICE because only the one generic type is expected and provided.

Fixes rust-lang#79559, fixes rust-lang#73584, fixes rust-lang#83117 (all duplicates)
Relevant to rust-lang#9307

r? ```@cjgillot```
Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 1, 2021
Fix ICE when `start` lang item has wrong generics

In my previous pr rust-lang#87875 I missed the requirements on the `start` lang item due to its relative difficulty to test and opting for more conservative estimates.  This fixes that by updating the requirement to be exactly one generic type.

The `start` lang item should have exactly one generic type for the return type of the `main` fn ptr passed to it.  I believe having zero would previously *sometimes* compile (often with the use of `fn() -> ()` as the fn ptr but it was likely UB to call if the return type of `main` was not `()` as far as I know) however it also sometimes would not for various errors including ICEs and LLVM errors depending on exact situations.  Having more than 1 generic has always failed with an ICE because only the one generic type is expected and provided.

Fixes rust-lang#79559, fixes rust-lang#73584, fixes rust-lang#83117 (all duplicates)
Relevant to rust-lang#9307

r? ````@cjgillot````
flip1995 pushed a commit to flip1995/rust that referenced this pull request Oct 7, 2021
Fix ICE when `start` lang item has wrong generics

In my previous pr rust-lang#87875 I missed the requirements on the `start` lang item due to its relative difficulty to test and opting for more conservative estimates.  This fixes that by updating the requirement to be exactly one generic type.

The `start` lang item should have exactly one generic type for the return type of the `main` fn ptr passed to it.  I believe having zero would previously *sometimes* compile (often with the use of `fn() -> ()` as the fn ptr but it was likely UB to call if the return type of `main` was not `()` as far as I know) however it also sometimes would not for various errors including ICEs and LLVM errors depending on exact situations.  Having more than 1 generic has always failed with an ICE because only the one generic type is expected and provided.

Fixes rust-lang#79559, fixes rust-lang#73584, fixes rust-lang#83117 (all duplicates)
Relevant to rust-lang#9307

r? ````@cjgillot````
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants