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

Make it a bit easier to debug TyKind::Error errors #70245

Closed
wants to merge 1 commit into from

Conversation

mark-i-m
Copy link
Member

r? @eddyb

Not sure how good of an idea this is... but it occurred to me that we can do more to enforce the invariant that TyKind::Error should only happen if errors are going to be emitted. We could also use the function to due a delay bug for good measure.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 21, 2020
src/librustc/ty/context.rs Outdated Show resolved Hide resolved
@eddyb
Copy link
Member

eddyb commented Mar 22, 2020

cc @rust-lang/compiler Let's get some visibility on this!

The other idea in this area is that we can make ErrorReported a token impossible to obtain without reporting an error (or using delay_span_bug I suppose), and then make ty::Error hold one, so that it acts as sort of a "proof".

@varkor
Copy link
Member

varkor commented Mar 22, 2020

The other idea in this area is that we can make ErrorReported a token impossible to obtain without reporting an error

This is a really nice idea 👍

@mark-i-m
Copy link
Member Author

@eddyb In order for that to be meaningful, we would need to not intern TyKind::Error in CommonTypes as we do currently. Do you know if there would be any performance effects of this?

@mark-i-m
Copy link
Member Author

I've pushed a rough prototype (not compiling yet, but I need to go to bed) with both of @eddyb's suggestions above. It's not clear that we need both, and between the two, I think prevention is better than detection, but I guess both can't hurt...

@rust-highfive
Copy link
Collaborator

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2020-03-22T04:14:12.7322586Z ========================== Starting Command Output ===========================
2020-03-22T04:14:12.7328392Z [command]/bin/bash --noprofile --norc /home/vsts/work/_temp/1e7aca6b-dafe-4b02-b15a-86d141f859b6.sh
2020-03-22T04:14:12.7328919Z 
2020-03-22T04:14:12.7333954Z ##[section]Finishing: Disable git automatic line ending conversion
2020-03-22T04:14:12.7356081Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/70245/merge to s
2020-03-22T04:14:12.7359763Z Task         : Get sources
2020-03-22T04:14:12.7360090Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-03-22T04:14:12.7360406Z Version      : 1.0.0
2020-03-22T04:14:12.7360621Z Author       : Microsoft
---
2020-03-22T04:14:13.7244421Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-03-22T04:14:13.7250165Z ##[command]git config gc.auto 0
2020-03-22T04:14:13.7254226Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-03-22T04:14:13.7258236Z ##[command]git config --get-all http.proxy
2020-03-22T04:14:13.7264942Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/70245/merge:refs/remotes/pull/70245/merge
---
2020-03-22T04:22:05.2634166Z     Checking rustc_infer v0.0.0 (/checkout/src/librustc_infer)
2020-03-22T04:22:05.6160210Z error[E0532]: expected unit struct, unit variant or constant, found tuple variant `ty::Error`
2020-03-22T04:22:05.6161256Z    --> src/librustc_infer/infer/sub.rs:120:15
2020-03-22T04:22:05.6161869Z     |
2020-03-22T04:22:05.6162647Z 120 |             (&ty::Error, _) | (_, &ty::Error) => {
2020-03-22T04:22:05.6163787Z     |               ^^^^^^^^^ did you mean `ty::Error( /* fields */ )`?
2020-03-22T04:22:05.6165315Z help: possible better candidates are found in other modules, you can import them into scope
2020-03-22T04:22:05.6165990Z     |
2020-03-22T04:22:05.6166652Z 1   | use core::fmt::Error;
2020-03-22T04:22:05.6167545Z     |
2020-03-22T04:22:05.6167545Z     |
2020-03-22T04:22:05.6168450Z 1   | use crate::traits::project::ProjectionCacheEntry::Error;
2020-03-22T04:22:05.6169186Z     |
2020-03-22T04:22:05.6169870Z 1   | use log::Level::Error;
2020-03-22T04:22:05.6170480Z     |
2020-03-22T04:22:05.6171158Z 1   | use log::LevelFilter::Error;
2020-03-22T04:22:05.6172256Z       and 6 other candidates
2020-03-22T04:22:05.6172509Z 
2020-03-22T04:22:05.6181385Z error[E0532]: expected unit struct, unit variant or constant, found tuple variant `ty::Error`
2020-03-22T04:22:05.6182294Z    --> src/librustc_infer/infer/sub.rs:120:36
2020-03-22T04:22:05.6182294Z    --> src/librustc_infer/infer/sub.rs:120:36
2020-03-22T04:22:05.6182885Z     |
2020-03-22T04:22:05.6183688Z 120 |             (&ty::Error, _) | (_, &ty::Error) => {
2020-03-22T04:22:05.6184892Z     |                                    ^^^^^^^^^ did you mean `ty::Error( /* fields */ )`?
2020-03-22T04:22:05.6186493Z help: possible better candidates are found in other modules, you can import them into scope
2020-03-22T04:22:05.6187142Z     |
2020-03-22T04:22:05.6187823Z 1   | use core::fmt::Error;
2020-03-22T04:22:05.6188434Z     |
2020-03-22T04:22:05.6188434Z     |
2020-03-22T04:22:05.6189214Z 1   | use crate::traits::project::ProjectionCacheEntry::Error;
2020-03-22T04:22:05.6189970Z     |
2020-03-22T04:22:05.6190643Z 1   | use log::Level::Error;
2020-03-22T04:22:05.6191252Z     |
2020-03-22T04:22:05.6191953Z 1   | use log::LevelFilter::Error;
2020-03-22T04:22:05.6193043Z       and 6 other candidates
2020-03-22T04:22:05.6193230Z 
2020-03-22T04:22:06.1845916Z error[E0599]: no method named `err` found for struct `rustc::ty::context::CommonTypes<'_>` in the current scope
2020-03-22T04:22:06.1847086Z     --> src/librustc_infer/infer/mod.rs:1694:63
2020-03-22T04:22:06.1847086Z     --> src/librustc_infer/infer/mod.rs:1694:63
2020-03-22T04:22:06.1847743Z      |
2020-03-22T04:22:06.1849522Z 1694 |             values: Types(ExpectedFound { expected: tcx.types.err(), found: tcx.types.err() }),
2020-03-22T04:22:06.1851907Z      |                                                               ^^^ method not found in `rustc::ty::context::CommonTypes<'_>`
2020-03-22T04:22:06.1878950Z error[E0599]: no method named `err` found for struct `rustc::ty::context::CommonTypes<'_>` in the current scope
2020-03-22T04:22:06.1880069Z     --> src/librustc_infer/infer/mod.rs:1694:87
2020-03-22T04:22:06.1881672Z      |
2020-03-22T04:22:06.1881672Z      |
2020-03-22T04:22:06.1882642Z 1694 |             values: Types(ExpectedFound { expected: tcx.types.err(), found: tcx.types.err() }),
2020-03-22T04:22:06.1884239Z      |                                                                                       ^^^ method not found in `rustc::ty::context::CommonTypes<'_>`
2020-03-22T04:22:06.2229937Z error[E0599]: no method named `err` found for struct `rustc::ty::context::CommonTypes<'_>` in the current scope
2020-03-22T04:22:06.2231111Z    --> src/librustc_infer/infer/canonical/mod.rs:157:44
2020-03-22T04:22:06.2231747Z     |
2020-03-22T04:22:06.2231747Z     |
2020-03-22T04:22:06.2232643Z 157 |                         ty: self.tcx.types.err(), // FIXME(const_generics)
2020-03-22T04:22:06.2233977Z     |                                            ^^^ method not found in `rustc::ty::context::CommonTypes<'_>`
2020-03-22T04:22:07.2160141Z error[E0599]: no method named `err` found for struct `rustc::ty::context::CommonTypes<'_>` in the current scope
2020-03-22T04:22:07.2161260Z    --> src/librustc_infer/infer/resolve.rs:193:38
2020-03-22T04:22:07.2161818Z     |
2020-03-22T04:22:07.2161818Z     |
2020-03-22T04:22:07.2162395Z 193 |                     self.tcx().types.err()
2020-03-22T04:22:07.2163378Z     |                                      ^^^ method not found in `rustc::ty::context::CommonTypes<'_>`
2020-03-22T04:22:07.2194233Z error[E0599]: no method named `err` found for struct `rustc::ty::context::CommonTypes<'_>` in the current scope
2020-03-22T04:22:07.2197866Z    --> src/librustc_infer/infer/resolve.rs:197:38
2020-03-22T04:22:07.2198559Z     |
2020-03-22T04:22:07.2198559Z     |
2020-03-22T04:22:07.2199277Z 197 |                     self.tcx().types.err()
2020-03-22T04:22:07.2200266Z     |                                      ^^^ method not found in `rustc::ty::context::CommonTypes<'_>`
2020-03-22T04:22:07.2257665Z error[E0599]: no method named `err` found for struct `rustc::ty::context::CommonTypes<'_>` in the current scope
2020-03-22T04:22:07.2258698Z    --> src/librustc_infer/infer/resolve.rs:201:38
2020-03-22T04:22:07.2259324Z     |
2020-03-22T04:22:07.2259324Z     |
2020-03-22T04:22:07.2260038Z 201 |                     self.tcx().types.err()
2020-03-22T04:22:07.2261266Z     |                                      ^^^ method not found in `rustc::ty::context::CommonTypes<'_>`
2020-03-22T04:22:07.2282043Z error[E0609]: no field `consts` on type `rustc::ty::TyCtxt<'_>`
2020-03-22T04:22:07.2282981Z    --> src/librustc_infer/infer/resolve.rs:234:39
2020-03-22T04:22:07.2283598Z     |
2020-03-22T04:22:07.2283598Z     |
2020-03-22T04:22:07.2284335Z 234 |                     return self.tcx().consts.err;
2020-03-22T04:22:07.2336620Z 
2020-03-22T04:22:07.2522437Z error[E0599]: no method named `err` found for struct `rustc::ty::context::CommonTypes<'_>` in the current scope
2020-03-22T04:22:07.2523751Z    --> src/librustc_infer/infer/sub.rs:122:37
2020-03-22T04:22:07.2524354Z     |
2020-03-22T04:22:07.2524354Z     |
2020-03-22T04:22:07.2525220Z 122 |                 Ok(self.tcx().types.err())
2020-03-22T04:22:07.2526516Z     |                                     ^^^ method not found in `rustc::ty::context::CommonTypes<'_>`
2020-03-22T04:22:07.3645345Z error: aborting due to 10 previous errors
2020-03-22T04:22:07.3645816Z 
2020-03-22T04:22:07.3651249Z Some errors have detailed explanations: E0532, E0599, E0609.
2020-03-22T04:22:07.3655254Z For more information about an error, try `rustc --explain E0532`.
2020-03-22T04:22:07.3655254Z For more information about an error, try `rustc --explain E0532`.
2020-03-22T04:22:07.3738683Z error: could not compile `rustc_infer`.
2020-03-22T04:22:07.3745997Z 
2020-03-22T04:22:07.3747216Z To learn more, run the command again with --verbose.
2020-03-22T04:22:07.3756730Z warning: build failed, waiting for other jobs to finish...
2020-03-22T04:22:08.1485158Z error: build failed
2020-03-22T04:22:08.1514595Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "2" "--release" "--color" "always" "--features" " llvm" "--manifest-path" "/checkout/src/rustc/Cargo.toml" "--message-format" "json-render-diagnostics"
2020-03-22T04:22:08.1529924Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check
2020-03-22T04:22:08.1530661Z Build completed unsuccessfully in 0:04:25
2020-03-22T04:22:08.1578496Z == clock drift check ==
2020-03-22T04:22:08.1593395Z   local time: Sun Mar 22 04:22:08 UTC 2020
2020-03-22T04:22:08.1593395Z   local time: Sun Mar 22 04:22:08 UTC 2020
2020-03-22T04:22:08.4494865Z   network time: Sun, 22 Mar 2020 04:22:08 GMT
2020-03-22T04:22:08.4498370Z == end clock drift check ==
2020-03-22T04:22:09.2548787Z 
2020-03-22T04:22:09.2626284Z ##[error]Bash exited with code '1'.
2020-03-22T04:22:09.2647660Z ##[section]Finishing: Run build
2020-03-22T04:22:09.2693943Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/70245/merge to s
2020-03-22T04:22:09.2699331Z Task         : Get sources
2020-03-22T04:22:09.2699635Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-03-22T04:22:09.2699914Z Version      : 1.0.0
2020-03-22T04:22:09.2700201Z Author       : Microsoft
2020-03-22T04:22:09.2700201Z Author       : Microsoft
2020-03-22T04:22:09.2700521Z Help         : [More Information](https://go.microsoft.com/fwlink/?LinkId=798199)
2020-03-22T04:22:09.2700876Z ==============================================================================
2020-03-22T04:22:09.6071255Z Cleaning any cached credential from repository: rust-lang/rust (GitHub)
2020-03-22T04:22:09.6125109Z ##[section]Finishing: Checkout rust-lang/rust@refs/pull/70245/merge to s
2020-03-22T04:22:09.6206139Z Cleaning up task key
2020-03-22T04:22:09.6207239Z Start cleaning up orphan processes.
2020-03-22T04:22:09.6378651Z Terminate orphan process: pid (3624) (python)
2020-03-22T04:22:09.6671988Z ##[section]Finishing: Finalize Job

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

@eddyb
Copy link
Member

eddyb commented Mar 22, 2020

I would call variables reported or error instead of proof, and have methods return it automatically without needing separate forms.
Also, can we change ErrorReported in a separate PR, before using it in Ty::Error?

Btw there an issue for the ErrorReported thing: #69426.

@bors
Copy link
Contributor

bors commented Mar 22, 2020

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

@mark-i-m
Copy link
Member Author

@eddyb

I would call the variable proof,

Which variable? I'm already using the name proof.

and have methods return it automatically without needing separate forms

Do you mean the methods of DiagnosticBuilder? The challenge is that we should only return ErrorProof if an error was emited, not just a warning or help.

Also, can we change ErrorReported in a separate PR, before using it in Ty::Error?

I had created a new ErrorProof type for this purpose. The problem is that a lot of other code is using ErrorReported already to just indicate an error condition in the code somewhere, not necessarily as we want to use it now.

@eddyb
Copy link
Member

eddyb commented Mar 22, 2020

Which variable? I'm already using the name proof.

Ugh, I accidentally a word. I meant to say I would call variables reported or error instead of proof. (edited the original comment as well)

The challenge is that we should only return ErrorProof if an error was emited, not just a warning or help.

Oh, is this about the "level" thing? Can we make that a const generic parameter of the DiagnosticBuilder or is that too extreme?

Ideally calling .emit() on a builder created by struct_span_err! should give ErrorReported.

I had created a new ErrorProof type for this purpose. The problem is that a lot of other code is using ErrorReported already to just indicate an error condition in the code somewhere, not necessarily as we want to use it now.

Ah, whereas I would start by changing ErrorReported, making all of its current usecases guaranteed to have errors reported, before I ever touch ty::Error.
This is not a priority and I'd rather us get it right from the start.

@mark-i-m
Copy link
Member Author

Oh, is this about the "level" thing? Can we make that a const generic parameter of the DiagnosticBuilder or is that too extreme?

Ideally calling .emit() on a builder created by struct_span_err! should give ErrorReported.

Hmm, I think I would rather not add a generic argument to all uses of DiagnosticBuilder. It seem like it would be infectious, but I might be wrong.

Another idea is to have something like:

enum ErrorReported {
  /// Emitted something with level < error
  Emitted,
  /// Emitted something with that will fail compilation
  EmittedWithProof(ErrorProof),
}

impl DiagnosticBuilder {
  pub fn emit(...) -> ErrorReported { ... }
}

I think this would accomodate both the current use cases and the ones where we want a proof.

@eddyb
Copy link
Member

eddyb commented Mar 22, 2020

ErrorReported is already used like a proof token, it's not just actually one.
AFAIK it's never used anywhere compilation could still succeed, that's sort of the point.

@eddyb
Copy link
Member

eddyb commented Mar 22, 2020

Hmm, I think I would rather not add a generic argument to all uses of DiagnosticBuilder.

Hmm, actually, a type parameter would make sense, whereas const generics would be hard to use. Only lints need variable level.

And most error reporting code doesn't name DiagnosticBuilder by name so this seems fine.

@mark-i-m
Copy link
Member Author

@eddyb so let me see if I understand correctly. Are you thinking of something like this:

// `true` if fails compilation
struct DiagnosticBuilder<const IS_ERROR: bool> {...}

impl DiagnosticBuilder<true> {
  pub fn emit() {
    // like today
  }
}

impl DiagnosticBuilder<false> {
  pub fn emit() -> Proof { ... } // returns proof
}

impl DiagnosticBuild<IS_ERROR> {
  // all the others remain generic
}

@mark-i-m
Copy link
Member Author

ErrorReported is already used like a proof token, it's not just actually one.
AFAIK it's never used anywhere compilation could still succeed, that's sort of the point.

I played around with this a bit yesterday, and I feel like I ran into cases where ErrorReported values were created in case they would be needed later. I can try to find them again if you want.

@eddyb
Copy link
Member

eddyb commented Mar 23, 2020

Forget I ever mentioned const generics (which I was imagining const LEVEL: Level).

What I have in mind most recently is this:

// Could even make this not constructible without calling
// a method that registers something similar to delay_bug.
// But that's overkill for now.
pub struct AlwaysError;

mod sealed {
	use super::{AlwaysError, ErrorReported, Level};

    pub trait LevelTrait {
        type Emitted;
        fn emit(self) -> (Level, Self::Emitted);
    }

    impl LevelTrait for Level {
        type Emitted = ();
        fn emit(self) -> (Level, Self::Emitted) {
            (self, ())
        }
    }

    impl LevelTrait for AlwaysError {
        type Emitted = ErrorReported;
        fn emit(self) -> (Level, Self::Emitted) {
            (Level::Error, ErrorReported { private: () })
        }
    }
}

struct DiagnosticBuilder<L> {
    level: L,
    ...
}

impl<L: sealed::LevelTrait> DiagnosticBuilder<L> {
    pub fn emit(self) -> L::Emitted {
        ...
        let (level, emitted) = self.level.emit();
        ...
        emitted
    }
}

@mark-i-m
Copy link
Member Author

Hmm... I could probably do something like that...

@mark-i-m
Copy link
Member Author

How would one deal with cancellation of errors? Ideally one would need to return the proof to cancel an error.

@mark-i-m
Copy link
Member Author

Hmm... ok this is getting very messy. There are too many weird edge cases. I still prefer the enum approach of #70245 (comment) .

(Also, I think we can just forget about cancellation for now. Anyway, we can do something like track_callers above to help debug weird cases where an error is cancelled.)

@mark-i-m
Copy link
Member Author

@eddyb after trying both, the enum approach is way easier to implement, largely because it doesn't clash with cancellation or lint levels; it just seems to be easier to do at runtime than compile time.

Also, I'm not sure what to do about a bunch of uses of types.err (please see the next rash of CI failures). Specifically, during type folding in a few places, we return types.err before the actual error is reported AFAICT, presumably to construct the error itself (e.g. src/librustc_infer/infer/resolve.rs:193)

@eddyb
Copy link
Member

eddyb commented Mar 23, 2020

I still think it's a waste of time to try to change ty::Error, it's far more disruptive than making ErrorReported correct-by-default separately.
If you want I can attempt what I'm suggesting once I'm done with #70164.

As for error cancellation, we should try to avoid that as much as possible.
Presumably a cancellable error wouldn't guarantee ErrorReported.
Actually, does cancelling happen before .emit()? Because that would be fine.

Specifically, during type folding in a few places, we return types.err before the actual error is reported AFAICT, presumably to construct the error itself (e.g. src/librustc_infer/infer/resolve.rs:193)

Ah, wait, inference contexts have snapshots, so you can try something and not actually report any errors. This might mean ty::Error can't hold any proof, or we'd need a different design.

@JohnCSimon JohnCSimon 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 Mar 28, 2020
@mark-i-m
Copy link
Member Author

As for error cancellation, we should try to avoid that as much as possible.
Presumably a cancellable error wouldn't guarantee ErrorReported.
Actually, does cancelling happen before .emit()? Because that would be fine.

AFAICT, a Diagnostic can be canceled at any time, even after emission. One idea is to make ErrorReported contain the Diagnostic itself. That way, if the Diagnostic is canceled, the ErrorReported must be revoked too.


Ok, for now, I'm going to pursue the track_caller idea you mentioned above, apart from the ErrorReported or ty::Error.

@mark-i-m mark-i-m deleted the ty-err branch May 6, 2020 01:54
RalfJung added a commit to RalfJung/rust that referenced this pull request Jun 18, 2020
Make all uses of ty::Error delay a span bug

r? @eddyb

A second attempt at rust-lang#70245

resolves rust-lang#70866
Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 18, 2020
Make all uses of ty::Error delay a span bug

r? @eddyb

A second attempt at rust-lang#70245

resolves rust-lang#70866
Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 18, 2020
Make all uses of ty::Error delay a span bug

r? @eddyb

A second attempt at rust-lang#70245

resolves rust-lang#70866
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants