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

Avoid going through the happy path in case of non-fn builtin calls #105973

Merged
merged 2 commits into from
Dec 21, 2022

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Dec 20, 2022

No functional change, just doing an early return. The removed comment is not applicable anymore, not every node needs type bindings in the error case. At best this would have been needed to avoid ICEs, but afaict this can't happen anymore today, as we do fallible checks.

@rustbot
Copy link
Collaborator

rustbot commented Dec 20, 2022

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jackh726 (or someone else) soon.

Please see the contribution instructions for more information.

@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 Dec 20, 2022
@jackh726
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Dec 20, 2022

📌 Commit 1c5b53b has been approved by jackh726

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 Dec 20, 2022
@compiler-errors
Copy link
Member

For the record, this is a functional change in the case that we have an additional type error inside one of the args.

@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 21, 2022

This PR and #105281 have the same diagnostic output on

fn main() -> Result<(), ()> {
    a(b(c(d(e(
        //~^ ERROR cannot find function `a` in this scope
        //~| ERROR cannot find function `b` in this scope
        //~| ERROR cannot find function `c` in this scope
        //~| ERROR cannot find function `d` in this scope
        //~| ERROR cannot find function `e` in this scope
        z????????????????????????????????????????????????????????????????????????????????????????
        ?????????????????????????????????????????????????????????????????????????????????????????
        ??????????????????????????????????????????????????????????????????
        //~^^^ ERROR cannot find value `z` in this scope
    )))))
}

fee1-dead added a commit to fee1-dead-contrib/rust that referenced this pull request Dec 21, 2022
…jackh726

Avoid going through the happy path in case of non-fn builtin calls

No functional change, just doing an early return. The removed comment is not applicable anymore, not every node needs type bindings in the error case. At best this would have been needed to avoid ICEs, but afaict this can't happen anymore today, as we do fallible checks.
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 21, 2022
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#105791 (docs: add long error explanation for error E0472)
 - rust-lang#105897 (Fix an opaque type ICE)
 - rust-lang#105904 (Fix arch flag on i686-apple-darwin)
 - rust-lang#105949 (Bump `cfg-if` to `1.0` in rustc crates)
 - rust-lang#105964 (rustdoc: prevent CSS layout of line numbers shrinking into nothing)
 - rust-lang#105972 (rustdoc: simplify section anchor CSS)
 - rust-lang#105973 (Avoid going through the happy path in case of non-fn builtin calls)
 - rust-lang#105976 (Remove unused `check-stage2-T-arm-linux-androideabi-H-x86_64-unknown-linux-gnu` make rule)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit b68e994 into rust-lang:master Dec 21, 2022
@rustbot rustbot added this to the 1.68.0 milestone Dec 21, 2022
@compiler-errors
Copy link
Member

This PR and #105281 have the same diagnostic output on [...]

That's actually just a coincidence of the extra check_expr call we do in suggest_call_as_method. We still unnecessarily skip checking later expressions for errors, for example:

fn main() -> Result<(), ()> {
    a(1, 1i32 == 2u32)
}

Goes from beta:

error[[E0425]](https://doc.rust-lang.org/beta/error-index.html#E0425): cannot find function `a` in this scope
 --> src/main.rs:2:5
  |
2 |     a(1, 1i32 == 2u32)
  |     ^ not found in this scope

error[[E0308]](https://doc.rust-lang.org/beta/error-index.html#E0308): mismatched types
 --> src/main.rs:2:18
  |
2 |     a(1, 1i32 == 2u32)
  |                  ^^^^ expected `i32`, found `u32`

error[[E0277]](https://doc.rust-lang.org/beta/error-index.html#E0277): can't compare `i32` with `u32`
 --> src/main.rs:2:15
  |
2 |     a(1, 1i32 == 2u32)
  |               ^^ no implementation for `i32 == u32`
  |
  = help: the trait `PartialEq<u32>` is not implemented for `i32`
  = help: the following other types implement trait `PartialEq<Rhs>`:
            <i32 as PartialEq<serde_json::Value>>
            <i32 as PartialEq<serde_yaml::Value>>
            <i32 as PartialEq>

To nightly:

error[[E0425]](https://doc.rust-lang.org/nightly/error-index.html#E0425): cannot find function `a` in this scope
 --> src/main.rs:2:5
  |
2 |     a(1, 1i32 == 2u32)
  |     ^ not found in this scope

I'm not happy that we do that, so I'll resurrect #105281 which calls check_expr properly on these arguments. I might also revert #106039 in that PR, since the only reason we need to return a ty_error is because we're not checking all of the arg expressions correctly...

@oli-obk oli-obk deleted the simplify_callee_checks branch December 22, 2022 18:15
Nilstrieb added a commit to Nilstrieb/rust that referenced this pull request Dec 23, 2022
…estebank

Check arg expressions properly on error in `confirm_builtin_call`

Makes sure we don't regress diagnostic output when we have an expr error nested inside of a bad fn call: rust-lang#105973 (comment)

Fixes rust-lang#106030
Fixes rust-lang#105244
Nilstrieb added a commit to Nilstrieb/rust that referenced this pull request Dec 23, 2022
…estebank

Check arg expressions properly on error in `confirm_builtin_call`

Makes sure we don't regress diagnostic output when we have an expr error nested inside of a bad fn call: rust-lang#105973 (comment)

Fixes rust-lang#106030
Fixes rust-lang#105244
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.

None yet

5 participants