Skip to content

Implement RFC 1260 with feature_name imported_main. #84401

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
Merged
merged 1 commit into from
Apr 30, 2021
Merged

Implement RFC 1260 with feature_name imported_main. #84401

merged 1 commit into from
Apr 30, 2021

Conversation

crlf0710
Copy link
Member

@crlf0710 crlf0710 commented Apr 21, 2021 •
edited by petrochenkov
Loading

This is the second extraction part of #84062 plus additional adjustments.
This (mostly) implements RFC 1260.

However there's still one test case failure in the extern crate case. Maybe LocalDefId doesn't work here? I'm not sure.

cc #28937
r? @petrochenkov

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 21, 2021
@crlf0710 crlf0710 force-pushed the impl_main_by_path branch 2 times, most recently from cbc1642 to 84a4ab3 Compare April 21, 2021 15:49
@crlf0710 crlf0710 mentioned this pull request Apr 21, 2021
Closed
crlf0710
crlf0710 commented Apr 21, 2021
View reviewed changes
@rust-log-analyzer

This comment has been minimized.

Sign in to view
petrochenkov
petrochenkov reviewed Apr 21, 2021
View reviewed changes
@crlf0710
Copy link
Member Author

crlf0710 commented Apr 22, 2021 •
edited
Loading

status update: still trying to find a way to migrate https://github.com/rust-lang/rust/blob/master/compiler/rustc_typeck/src/lib.rs#L167 to support non-local DefIds. Searched around without much findings. Created a zulip stream at https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/How.20to.20typecheck.20non-local.20DefId.3F

@crlf0710 crlf0710 force-pushed the impl_main_by_path branch from 84a4ab3 to 2d4c666 Compare April 24, 2021 02:27
@crlf0710
Copy link
Member Author

Status update: Updated the types and modified rustc_typeck to take care of the non-local case.

Currently still a linking error with the non-local case...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@crlf0710 crlf0710 force-pushed the impl_main_by_path branch from 2d4c666 to 1eff17f Compare April 24, 2021 02:35
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@crlf0710
Copy link
Member Author

Added a mechanism to ensure there's a codegen unit that's appointed to deal with the main_wrapper generation.

cc @bjorn3 in case there's potential effect on cranelift codegen.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
bjorn3
bjorn3 reviewed Apr 24, 2021
View reviewed changes
@bjorn3 bjorn3 mentioned this pull request Apr 24, 2021
Closed
@petrochenkov
Copy link
Contributor

I can't help with codegen issues with foreign main.
As one possible solution I can suggest reporting an error if the imported main points to a different crate in this PR, and then implementing support for it in a separate PR assigned to someone more familiar with the relevant compiler parts.

petrochenkov
petrochenkov reviewed Apr 24, 2021
View reviewed changes
@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 Apr 24, 2021
@bors

This comment has been minimized.

Sign in to view
@crlf0710 crlf0710 changed the title Implement RFC 1260 with feature_name main_by_path. Implement RFC 1260 with feature_name imported_main. Apr 25, 2021
@crlf0710 crlf0710 force-pushed the impl_main_by_path branch from 5ee043c to 7d7c553 Compare April 25, 2021 18:01
@crlf0710
Copy link
Member Author

Rebased and addressed the review comments. I'll leave the codegen changes to another PR.

Surprisingly, the last-seen CI-failure is actually not caused by codegen, but by the previously used resolve_ast_path api. Not sure why it caused such issues, but we're no longer using it anyway.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
petrochenkov
petrochenkov reviewed Apr 25, 2021
View reviewed changes
@crlf0710 crlf0710 mentioned this pull request Apr 26, 2021
Closed
@crlf0710 crlf0710 force-pushed the impl_main_by_path branch from c0fe1d3 to 9182f04 Compare April 28, 2021 02:21
@crlf0710
Copy link
Member Author

crlf0710 commented Apr 28, 2021 •
edited
Loading

Rebased and address the comments above in a new commit for easier reviewing.

@crlf0710 crlf0710 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 Apr 28, 2021
@petrochenkov
Copy link
Contributor

r=me with commits squashed.

@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 Apr 28, 2021
@crlf0710 crlf0710 force-pushed the impl_main_by_path branch from 9182f04 to d261df4 Compare April 29, 2021 00:35
@crlf0710
Copy link
Member Author

Squashed the commits.

@crlf0710 crlf0710 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 Apr 29, 2021
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Apr 29, 2021

📌 Commit d261df4 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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 29, 2021
@bors
Copy link
Collaborator

bors commented Apr 30, 2021

⌛ Testing commit d261df4 with merge bcd696d...

@bors
Copy link
Collaborator

bors commented Apr 30, 2021

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing bcd696d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 30, 2021
@bors bors merged commit bcd696d into rust-lang:master Apr 30, 2021
@rustbot rustbot added this to the 1.53.0 milestone Apr 30, 2021
@rust-highfive rust-highfive mentioned this pull request Apr 30, 2021
Closed
@rust-highfive
Copy link
Contributor

📣 Toolstate changed by #84401!

Tested on commit bcd696d.
Direct link to PR: #84401

💔 miri on windows: test-pass → build-fail (cc @RalfJung @eddyb @oli-obk).
💔 miri on linux: test-pass → build-fail (cc @RalfJung @eddyb @oli-obk).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Apr 30, 2021
@rust-highfive
📣 Toolstate changed by rust-lang/rust#84401!
ee78eae 
Tested on commit rust-lang/rust@bcd696d.
Direct link to PR: <rust-lang/rust#84401>

💔 miri on windows: test-pass → build-fail (cc @RalfJung @eddyb @oli-obk).
💔 miri on linux: test-pass → build-fail (cc @RalfJung @eddyb @oli-obk).
@crlf0710 crlf0710 deleted the impl_main_by_path branch April 30, 2021 11:42
bors added a commit to rust-lang/miri that referenced this pull request Apr 30, 2021
@bors
Auto merge of #1783 - hyd-dev:rustup, r=RalfJung
41f3fe6 
Rustup for rust-lang/rust#84401

cc rust-lang/rust#84741
flip1995 pushed a commit to flip1995/rust that referenced this pull request May 6, 2021
@bors
Auto merge of rust-lang#84401 - crlf0710:impl_main_by_path, r=petroch…
5491c80 
…enkov

Implement RFC 1260 with feature_name `imported_main`.

This is the second extraction part of rust-lang#84062 plus additional adjustments.
This (mostly) implements RFC 1260.

However there's still one test case failure in the extern crate case. Maybe `LocalDefId` doesn't work here? I'm not sure.

cc rust-lang#28937
r? `@petrochenkov`
This was referenced Jun 9, 2021
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 1, 2021
@bors
Auto merge of rust-lang#86190 - asquared31415:extern-main-86110-fix, …
f8ac8fd 
…r=varkor

Fix ICE when `main` is declared in an `extern` block

Changes in rust-lang#84401 to implement `imported_main` changed how the crate entry point is found, and a declared `main` in an `extern` block was detected erroneously.  This was causing the ICE described in rust-lang#86110.

This PR adds a check for this case and emits an error instead.  Previously a `main` declaration in an `extern` block was not detected as an entry point at all, so emitting an error shouldn't break anything that worked previously.  In 1.52.1 stable this is demonstrated, with a `` `main` function not found`` error.

Fixes rust-lang#86110
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

@bjorn3 bjorn3 bjorn3 left review comments

Assignees

@petrochenkov petrochenkov

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.

Projects

None yet

Milestone

1.53.0

Development

Successfully merging this pull request may close these issues.

7 participants

@crlf0710 @rust-log-analyzer @petrochenkov @bors @rust-highfive @bjorn3 @rustbot