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

Add new rpitit compare mode #108710

Closed

Conversation

spastorino
Copy link
Member

@spastorino spastorino commented Mar 3, 2023

r? @compiler-errors

This depends on #108700, after that one is merged I can rebase it on top of master but for now is good for a review. The last commit 7dfcdca is the only meaningful one, the rest are about #108700

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc 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 Mar 3, 2023
@jackh726
Copy link
Member

jackh726 commented Mar 3, 2023

I don't see the benefit of a separate compare mode, vs being explicit. Compare modes are more useful for things that will every every program. RPITIT lowering does not.

@compiler-errors
Copy link
Member

Yeah, @jackh726 is right. We can probably just use revisions for all the tests in impl-trait/in-trait and async-await/in-trait. It would be cool if we could have opt-in migrate mode instead of opt-out, but I digress.

@spastorino
Copy link
Member Author

@jackh726 I'm not sure what did you mean exactly, but we are aiming to replace existing code with what we have under -Zlower-impl-trait-in-trait-to-assoc-ty. I think this is useful in order to:

  1. Disallow people to push code that breaks under lower-impl-trait-in-trait-to-assoc-ty
  2. As a way to track failing tests (which can be done also just using revisions)

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check-tidy failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
Prepare all required actions
Getting action download info
Download action repository 'actions/checkout@v3' (SHA:ac593985615ec2ede58e132d2e21d2b1cbd6127c)
Download action repository 'rust-lang/simpleinfra@master' (SHA:0fdabd83e1d3faaa8e9cfd7c00031e3a92997344)
Complete job name: PR (mingw-check-tidy, true, ubuntu-20.04-xl)
git config --global core.autocrlf false
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
  CI_JOB_NAME: mingw-check-tidy
---
Building wheels for collected packages: reuse
  Building wheel for reuse (pyproject.toml): started
  Building wheel for reuse (pyproject.toml): finished with status 'done'
  Created wheel for reuse: filename=reuse-1.1.0-cp310-cp310-manylinux_2_35_x86_64.whl size=180118 sha256=2f37b3a1d0677cd3228b6c9d3baf1ad4a6f9d44f04404765e8544fc47767cbcc
  Stored in directory: /tmp/pip-ephem-wheel-cache-a4e5rlho/wheels/c2/3c/b9/1120c2ab4bd82694f7e6f0537dc5b9a085c13e2c69a8d0c76d
Installing collected packages: boolean-py, binaryornot, setuptools, reuse, python-debian, markupsafe, license-expression, jinja2, chardet
  Attempting uninstall: setuptools
    Found existing installation: setuptools 59.6.0
    Not uninstalling setuptools at /usr/lib/python3/dist-packages, outside environment /usr
---
Successfully built 35f0492f8e36
Successfully tagged rust-ci:latest
Built container sha256:35f0492f8e363f5b59cd658b44cc35297a4abbd54b17d53fd4d60eed2ba386b1
Uploading finished image to https://ci-caches.rust-lang.org/docker/38eba79578bd6d379f830cfbe873c54dad8d10db1dfcf4fbac8af27e25e93a7ecaa2a7c62028c216d50741db91fde3d04f83489b4d576287b0239d35dee6e197
upload failed: - to s3://rust-lang-ci-sccache2/docker/38eba79578bd6d379f830cfbe873c54dad8d10db1dfcf4fbac8af27e25e93a7ecaa2a7c62028c216d50741db91fde3d04f83489b4d576287b0239d35dee6e197 Unable to locate credentials
[CI_JOB_NAME=mingw-check-tidy]
[CI_JOB_NAME=mingw-check-tidy]
---
tidy check
tidy: Skipping binary file check, read-only filesystem
Found 505 error codes
Highest error code: `E0793`
tidy error: following path contains more than 940 entries, you should move the test to some relevant subdirectory (current: 941): /checkout/tests/ui
tidy error: following path contains more than 1978 entries, you should move the test to some relevant subdirectory (current: 1982): /checkout/tests/ui/issues
some tidy checks failed
Build completed unsuccessfully in 0:00:23

@jackh726
Copy link
Member

jackh726 commented Mar 4, 2023

Yes, but this compiler flag should only affect RPITIT lowering, and thus only the tests in the directories listed by @compiler-errors.

To argue some more against this: RPITITs are unstable now anyways. I doubt we want different behavior for long. Revisions are enough if needed. I might even argue that it's not even worth adding the compile flag and just making the change (you want this to get tested, and having a flag for an unstable feature seems counter to that goal).

@compiler-errors
Copy link
Member

I might even argue that it's not even worth adding the compile flag and just making the change (you want this to get tested, and having a flag for an unstable feature seems counter to that goal).

I disagree. I don't want to break the very real projects that are using AFIT experimentally today, which will happen if we were to land a "totally rewrite the way we do RPITIT lowering" PR. A rewrite at this scale really needs to be landed in chunks with lots of implementation discussion, and the only way of doing that is gating the feature behind a flag until at least it has UI test parity with the existing implementation.

@spastorino
Copy link
Member Author

It's true that most tests live where @compiler-errors mentioned (note that there are a ton of other places where we use RPITITs and AFITs trying to test other stuff). Anyway, I can add revisions to those and be sure that I keep track of the ones that are working and the ones that are not working.
The reason for me for opening this PR was to be sure that we are not pushing code that break things related to this change. Anyway, the compare-mode and the unstable flag are temporary things until we are sure so IMO this shouldn't hurt.
I'm also fine with closing this and I can add revisions if you both prefer.

@bors
Copy link
Contributor

bors commented Mar 5, 2023

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

@spastorino
Copy link
Member Author

Closing in favor of using revisions.

@spastorino spastorino closed this Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants