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

Split detecting unconstructible pub structs into a new lint from dead_code #128389

Closed
wants to merge 1 commit into from

Conversation

mu001999
Copy link
Contributor

@mu001999 mu001999 commented Jul 30, 2024

#125572 detects never constructed pub structs, but introduces some regressions (#126169 and #128272).

This does not fix the regressions but just split the detection into a new lint, #127104 and #128329 try to fix them.

I don't know if it is late or not for this PR. But this could at least provide a way to help silence the lint separately for current or potential future regressions.

@rustbot
Copy link
Collaborator

rustbot commented Jul 30, 2024

r? @nnethercote

rustbot has assigned @nnethercote.
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 Jul 30, 2024
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@mu001999
Copy link
Contributor Author

mu001999 commented Jul 30, 2024

wired failure:

   3 warnings found (use docker --debug to expand):
   - LegacyKeyValueFormat: "ENV key=value" should be used instead of legacy "ENV key value" format (line 81)
   - LegacyKeyValueFormat: "ENV key=value" should be used instead of legacy "ENV key value" format (line 86)
   - LegacyKeyValueFormat: "ENV key=value" should be used instead of legacy "ENV key value" format (line 101)
  Dockerfile:76
  --------------------
    74 |     
    75 |     COPY scripts/nodejs.sh /scripts/
    76 | >>> RUN sh /scripts/nodejs.sh /node
    77 |     ENV PATH="/node/bin:${PATH}"
    78 |     
  --------------------
  ERROR: failed to solve: process "/bin/sh -c sh /scripts/nodejs.sh /node" did not complete successfully: exit code: 2
  The command has failed after 5 attempts.
  Error: Process completed with exit code 1.

🤔is there any other way to manually restart gh jobs other than push new?

Comment on lines 1186 to 1192
let lint = if tcx.visibility(first_item.def_id).is_public()
&& matches!(tcx.def_kind(first_item.def_id), DefKind::Struct)
{
UNCONSTRUCTIBLE_PUB_STRUCT
} else {
DEAD_CODE
};
Copy link
Member

Choose a reason for hiding this comment

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

It seems very sketchy that this depends on first_item. Doesn't this suppress dead_code if a pub ADT comes first in the list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems very sketchy that this depends on first_item. Doesn't this suppress dead_code if a pub ADT comes first in the list?

No, we will only have one pub struct there. Because this function will only be called by warn_dead_code for struct.

@rust-log-analyzer

This comment has been minimized.

@nnethercote
Copy link
Contributor

I will assign to @compiler-errors because he has already started reviewing.

r? @compiler-errors

@compiler-errors
Copy link
Member

This PR has the side-effect of now suppressing dead_code warnings if we have unreachable-pub structs, such as mod x { pub Unreachable(i32); }. I think the new lint should only trigger for the new dead_code behavior introduced recently.

@mu001999
Copy link
Contributor Author

mu001999 commented Jul 31, 2024

@compiler-errors updated and use tcx.effective_visibilities(()).is_reachable now, and add a test for mod x { pub Unreachable(i32); } in tests/ui/lint/dead-code/unconstructible-pub-struct.rs

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Aug 3, 2024

☔ The latest upstream changes (presumably #128404) made this pull request unmergeable. Please resolve the merge conflicts.

@compiler-errors
Copy link
Member

Let's reintroduce this lint later. I will leave it up to you, but this is probably best closed.

@rustbot author

@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 Aug 5, 2024
@mu001999 mu001999 closed this Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

6 participants