Skip to content

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Apr 12, 2024

I was wondering why I couldn't come up with a test that hits the code path of the argument check checking the types we inferred from the return type... Turns out we reject those attempts early during fudging.

I have absolutely no information for you as to what kind of type inference changes this may incur, but I think we should just land this out of two reasons:

  • had I found the other place to use opaque type inference on before I added the hack, we'd be using that today and this PR would never have happened
  • if it is possible to hit this path, it requires some god awful recursive RPIT logic that I doubt anyone would have written without actively trying to write obscure code

r? @ghost

@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 Apr 12, 2024
@oli-obk
Copy link
Contributor Author

oli-obk commented Apr 12, 2024

@bors try

let's still crater it

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 12, 2024
Remove a HACK by instead inferring opaque types during expected/formal type checking

I was wondering why I couldn't come up with a test that hits the code path of the argument check checking the types we inferred from the return type... Turns out we reject those attempts early during fudging.

I have absolutely no information for you as to what kind of type inference changes this may incur, but I think we should just land this out of two reasons:

* had I found the other place to use opaque type inference on before I added the hack, we'd be using that today and this PR would never have happened
* if it is possible to hit this path, it requires some god awful recursive RPIT logic that I doubt anyone would have written without actively trying to write obscure code

r? `@ghost`
@bors
Copy link
Collaborator

bors commented Apr 12, 2024

⌛ Trying commit 24653a5 with merge c0f799a...

@bors
Copy link
Collaborator

bors commented Apr 12, 2024

☀️ Try build successful - checks-actions
Build commit: c0f799a (c0f799aeda763fc507814da0dc46c2dde7ca9133)

@oli-obk
Copy link
Contributor Author

oli-obk commented Apr 12, 2024

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-123864 created and queued.
🤖 Automatically detected try build c0f799a
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 12, 2024
@craterbot
Copy link
Collaborator

🚧 Experiment pr-123864 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-123864 is completed!
📊 6 regressed and 2 fixed (437394 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Apr 14, 2024
@oli-obk
Copy link
Contributor Author

oli-obk commented Apr 15, 2024

All of the regressions are spurious (either repro on master or are just disk out of space)

@oli-obk
Copy link
Contributor Author

oli-obk commented Apr 15, 2024

r? @compiler-errors

@compiler-errors
Copy link
Member

Yeah, I'm happy with landing this now.

@bors r+

@bors
Copy link
Collaborator

bors commented Apr 15, 2024

📌 Commit 24653a5 has been approved by compiler-errors

It is now in the queue for this repository.

@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 15, 2024
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Apr 15, 2024
…mpiler-errors

Remove a HACK by instead inferring opaque types during expected/formal type checking

I was wondering why I couldn't come up with a test that hits the code path of the argument check checking the types we inferred from the return type... Turns out we reject those attempts early during fudging.

I have absolutely no information for you as to what kind of type inference changes this may incur, but I think we should just land this out of two reasons:

* had I found the other place to use opaque type inference on before I added the hack, we'd be using that today and this PR would never have happened
* if it is possible to hit this path, it requires some god awful recursive RPIT logic that I doubt anyone would have written without actively trying to write obscure code

r? `@ghost`
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 15, 2024
Rollup of 12 pull requests

Successful merges:

 - rust-lang#123423 (Distribute LLVM bitcode linker as a preview component)
 - rust-lang#123548 (libtest: also measure time in Miri)
 - rust-lang#123666 (Fix some typos in doc)
 - rust-lang#123864 (Remove a HACK by instead inferring opaque types during expected/formal type checking)
 - rust-lang#123896 (Migrate some diagnostics in `rustc_resolve` to session diagnostic)
 - rust-lang#123919 (builtin-derive: tag → discriminant)
 - rust-lang#123922 (Remove magic constants when using `base_n`.)
 - rust-lang#123931 (Don't leak unnameable types in `-> _` recover)
 - rust-lang#123933 (move the LargeAssignments lint logic into its own file)
 - rust-lang#123934 (`rustc_data_structures::graph` mini refactor)
 - rust-lang#123941 (Fix UB in LLVM FFI when passing zero or >1 bundle)
 - rust-lang#123957 (disable create_dir_all_bare test on all(miri, windows))

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 699612f into rust-lang:master Apr 15, 2024
@rustbot rustbot added this to the 1.79.0 milestone Apr 15, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 15, 2024
Rollup merge of rust-lang#123864 - oli-obk:define_opaque_types3, r=compiler-errors

Remove a HACK by instead inferring opaque types during expected/formal type checking

I was wondering why I couldn't come up with a test that hits the code path of the argument check checking the types we inferred from the return type... Turns out we reject those attempts early during fudging.

I have absolutely no information for you as to what kind of type inference changes this may incur, but I think we should just land this out of two reasons:

* had I found the other place to use opaque type inference on before I added the hack, we'd be using that today and this PR would never have happened
* if it is possible to hit this path, it requires some god awful recursive RPIT logic that I doubt anyone would have written without actively trying to write obscure code

r? ``@ghost``
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

5 participants