Fix several bugs in ferrocene::prevalidated#2336
Conversation
284b25b to
c24f42b
Compare
c24f42b to
86ce85e
Compare
Hoverbear
left a comment
There was a problem hiding this comment.
Overall looking good, we discussed internally that we'd like to investigate more the safety of disabling the inline_mir option.
86ce85e to
db6fc38
Compare
I've investigated this. I've found that disabling |
Urhengulas
left a comment
There was a problem hiding this comment.
Seems good, although I cannot fully assess if the compiler/lint code is correct. I also tested it on the library I was using it and it now successfully detects the unvalidated functions that it did not detect before.
|
This PR affects coverage like following: Executing
The lower number for partially tested is odd... |
|
🕐 Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set. |
2291: backport: squash and use `cherry-pick`, not rebase r=Urhengulas a=jyn514 This hits many fewer conflicts when backporting. Now, instead of requiring every commit to backport cleanly, we only require the diff between `main` and the PR to backport cleanly. This decreases the fraction of PRs that need to be submitted manually, and also reduces the manual work needed to backport PRs when a manual backport is needed. There are also various other changes to make the backporting scripts more maintainable and testable; see each individual commit for details. Unlike before, this does not mess with the current git state; running `git cherry-pick --abort` puts you back in exactly the same state you were in before you ran `one.py`. See #2301 for an example PR opened by this automation, and https://github.com/ferrocene/ferrocene/actions/runs/24351997960/job/71108663815 for a sample job output. --- ## Testing Here is a sample session using `one.py` to do a manual backport for the upcoming release: ```console $ git checkout origin/release/1.95 HEAD is now at a31dd5c Merge #2258 $ git checkout jyn/backport-automation ferrocene/tools/backport ferrocene/tools/automations-common $ ferrocene/tools/backport/one.py 2245 ERROR: this script cannot be run when you have staged changes; try running `git reset` $ git reset Unstaged changes after reset: M ferrocene/tools/automations-common/src/automations_common/automated_prs.py M ferrocene/tools/backport/all.py M ferrocene/tools/backport/one.py $ ferrocene/tools/backport/one.py 2245 17:45 warning: if no API token is set in the GITHUB_TOKEN env var, requests may be rate-limited INFO: backporting #2245 to a31dd5c (please do not CTRL-C) Auto-merging .circleci/workflows.yml Auto-merging ferrocene/ci/scripts/setup-uv.sh CONFLICT (content): Merge conflict in ferrocene/ci/scripts/setup-uv.sh error: could not apply d3faae633305... Backport #2245 ERROR: Encountered a merge conflict backporting #2245 INFO: attempting to auto-solve conflicts with mergiraf WARN Mergiraf: Could not find a supported language for ferrocene/ci/scripts/setup-uv.sh HELP: Fix the conflicts, run 'git add/rm <filepath>', then run 'git cherry-pick --continue'. NOTE: Running 'git cherry-pick --abort' will put you back in your original git state. NOTE: conflicts exist in the following locations: ferrocene/ci/scripts/setup-uv.sh:7-17 NOTE: 'HEAD' in the conflicted files refers to your current branch, a31dd5c $ git diff --name-status --cached M .circleci/workflows.yml U ferrocene/ci/scripts/setup-uv.sh $ git status | head -n5 HEAD detached at origin/release/1.95 You are currently cherry-picking commit d3faae633305. (fix conflicts and run "git cherry-pick --continue") (use "git cherry-pick --skip" to skip this patch) (use "git cherry-pick --abort" to cancel the cherry-pick operation) ``` (After this lands on beta, the `git checkout ferrocene/tools && git reset` lines will no longer be needed.) 2336: Fix several bugs in `ferrocene::prevalidated` r=Urhengulas a=jyn514 Thanks to `@Urhengulas` for catching these. 1. The THIR pass didn't check associated items at all. This is fixed in the first commit and tested in `tests/ui/ferrocene/lint-prevalidated/thir-impl-item.rs`. The second commit adds the missing items to libcore. 2. The post-mono pass didn't check dynamic casts. I had correctly "failed closed" here -- the THIR pass would ICE if it wasn't able to check a cast — but because of 1. there were several casts in libcore that weren't caught, including in libcore. The ICE is fixed in the third commit and the post-mono pass is extended in the fourth. This is tested in `tests/ui/ferrocene/lint-prevalidated/dyn-cast-post-mono.rs`. 3. The post-mono pass can't handle MIR inlining. I tried to get this working (see https://rust-lang.zulipchat.com/#narrow/channel/182449-t-compiler.2Fhelp/topic/SourceScopeData.20-.3E.20SourceScope/with/590200295), but wasn't able to figure out how to do it reliably. Instead I just disable mir inlining in the 5th commit. This is tested in `tests/ui/ferrocene/lint-prevalidated/mir-inlining.rs`. Co-authored-by: Jynn Nelson <[email protected]> Co-authored-by: Urhengulas <[email protected]>
|
Build failed (retrying...): |
2336: Fix several bugs in `ferrocene::prevalidated` r=Urhengulas a=jyn514 Thanks to `@Urhengulas` for catching these. 1. The THIR pass didn't check associated items at all. This is fixed in the first commit and tested in `tests/ui/ferrocene/lint-prevalidated/thir-impl-item.rs`. The second commit adds the missing items to libcore. 2. The post-mono pass didn't check dynamic casts. I had correctly "failed closed" here -- the THIR pass would ICE if it wasn't able to check a cast — but because of 1. there were several casts in libcore that weren't caught, including in libcore. The ICE is fixed in the third commit and the post-mono pass is extended in the fourth. This is tested in `tests/ui/ferrocene/lint-prevalidated/dyn-cast-post-mono.rs`. 3. The post-mono pass can't handle MIR inlining. I tried to get this working (see https://rust-lang.zulipchat.com/#narrow/channel/182449-t-compiler.2Fhelp/topic/SourceScopeData.20-.3E.20SourceScope/with/590200295), but wasn't able to figure out how to do it reliably. Instead I just disable mir inlining in the 5th commit. This is tested in `tests/ui/ferrocene/lint-prevalidated/mir-inlining.rs`. Co-authored-by: Jynn Nelson <[email protected]> Co-authored-by: Urhengulas <[email protected]>
|
Build failed:
|
f264a88 to
565fa40
Compare
Previously this wouldn't get caught until the post-mono pass, which made it annoying to check whether generic inline functions were validated.
- UnresolvedGenericImpl is unreachable outside THIR - fix typo
plus other minor fixes
565fa40 to
3a7e4b8
Compare
|
bors try |
@jyn514 I think this is unintentional?
|
tryBuild failed: |
a80d0e1 to
c27f4d1
Compare
|
bors try |
tryBuild failed: |
These weren't tested before, since we only generated our report for x86_64-linux-gnu.
|
bors try |
tryBuild failed: |
c27f4d1 to
2592e12
Compare
|
bors try |
tryBuild failed: |
|
the fail is because i did |
|
Build succeeded:
|
Fix several bugs in `ferrocene::prevalidated` Thanks to @Urhengulas for catching these. 1. The THIR pass didn't check associated items at all. This is fixed in the first commit and tested in `tests/ui/ferrocene/lint-prevalidated/thir-impl-item.rs`. The second commit adds the missing items to libcore. 2. The post-mono pass didn't check dynamic casts. I had correctly "failed closed" here -- the THIR pass would ICE if it wasn't able to check a cast — but because of 1. there were several casts in libcore that weren't caught, including in libcore. The ICE is fixed in the third commit and the post-mono pass is extended in the fourth. This is tested in `tests/ui/ferrocene/lint-prevalidated/dyn-cast-post-mono.rs`. 3. The post-mono pass can't handle MIR inlining. ~~I tried to get this working (see https://rust-lang.zulipchat.com/#narrow/channel/182449-t-compiler.2Fhelp/topic/SourceScopeData.20-.3E.20SourceScope/with/590200295), but wasn't able to figure out how to do it reliably. Instead I just disable mir inlining in the 5th commit.~~ I got this working, and added a test in `tests/ui/ferrocene/lint-prevalidated/mir-inlining.rs`.Co-authored-by: {author} Ferrocene-backport-of: #2336 Ferrocene-backported-commits: 2592e12 fdea74a 3a7e4b8 5a3e3bd 0c9007d dcfd52b 383e22d 4c7293c 9d9681c

Thanks to @Urhengulas for catching these.
tests/ui/ferrocene/lint-prevalidated/thir-impl-item.rs. The second commit adds the missing items to libcore.tests/ui/ferrocene/lint-prevalidated/dyn-cast-post-mono.rs.I tried to get this working (see https://rust-lang.zulipchat.com/#narrow/channel/182449-t-compiler.2Fhelp/topic/SourceScopeData.20-.3E.20SourceScope/with/590200295), but wasn't able to figure out how to do it reliably. Instead I just disable mir inlining in the 5th commit.I got this working, and added a test intests/ui/ferrocene/lint-prevalidated/mir-inlining.rs.