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

Type inference regression #38535

Closed
SimonSapin opened this issue Dec 22, 2016 · 15 comments · Fixed by #38566 or servo/servo#14689
Closed

Type inference regression #38535

SimonSapin opened this issue Dec 22, 2016 · 15 comments · Fixed by #38566 or servo/servo#14689
Assignees
Labels
A-typesystem Area: The type system P-high High priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@SimonSapin
Copy link
Contributor

Servo compiles with rustc 71c06a5 2016-12-18, but not with 439c312 2016-12-21. Diff: 71c06a5...439c312

The style crate fails to build with:

error[E0282]: unable to infer enough type information about `E`
  --> /home/simon/projects/servo/components/style/font_face.rs:65:9
   |
65 |         try!(self.family.to_css(dest));
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot infer type for `E`
   |
   = note: type annotations or generic parameter binding required
   = note: this error originates in a macro outside of the current crate

The to_css method being called here is:

impl ToCss for FontFamily {
    fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
        // ...
    }
}

Its return type is std::fmt::Result which is type Result = std::result::Result<(), std::fmt::Error>;. This call is in another impl of the same method of the same trait:

impl ToCss for FontFaceRule {
    fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
        try!(dest.write_str("@font-face { font-family: "));
        try!(self.family.to_css(dest));
        try!(dest.write_str(";"));

        // ...
    }
}

Adding an explicit type annotation works around this error, but only to uncover another very similar one, then another. The third one was in code generated by serde’s derive(Serialize), so I gave up that approach.

        let r: fmt::Result = self.family.to_css(dest);
        try!(r);

So far I haven’t managed to reduce a test case smaller than "most of Servo", sorry.

I’m suspicious of 6ea1fbb (which is part of #38099) because it’s commit message mentions inference. I’m not building a compiler #38099 to confirm.

@SimonSapin
Copy link
Contributor Author

CC @GuillaumeGomez, @nikomatsakis

@SimonSapin
Copy link
Contributor Author

Also fails in 92d4600, so #38099 is not to blame. New diff: 71c06a5...92d4600

I don’t have another guess, and doing a full bisect would take a very long time…

@eddyb
Copy link
Member

eddyb commented Dec 22, 2016

I can't find the cause in that range, at least nothing changed in the compiler AFAICT.
Can you try 3038f30 (the commit before @alexcrichton's larger rollup)?

@SimonSapin
Copy link
Contributor Author

Expanding the try! macro:

diff --git a/components/style/font_face.rs b/components/style/font_face.rs
index 70f904c17a..a3d0ca3882 100644
--- a/components/style/font_face.rs
+++ b/components/style/font_face.rs
@@ -62,7 +62,10 @@ impl ToCss for FontFaceRule {
     // Serialization of FontFaceRule is not specced.
     fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
         try!(dest.write_str("@font-face { font-family: "));
-        try!(self.family.to_css(dest));
+        match self.family.to_css(dest) {
+            Err(err) => return Err(err),
+            Ok(_) => {}
+        };
         try!(dest.write_str(";"));
 
         if self.sources.len() > 0 {

(Even simplifying it, this is not using From) gives:

error[E0282]: unable to infer enough type information about `T`
  --> /home/simon/projects/servo2/components/style/font_face.rs:66:13
   |
66 |             Err(err) => return Err(err),
   |             ^^^^^^^^ cannot infer type for `T`
   |
   = note: type annotations or generic parameter binding required

error: aborting due to previous error

@eddyb
Copy link
Member

eddyb commented Dec 22, 2016

@SimonSapin Alright, that matches what I've seen from @nox.
What about a couple more steps of bisecting? Or maybe compiling this crate with the "servo" feature off
(which, AFAICT, turns off all the macros 1.1 - they might be responsible).

@SimonSapin
Copy link
Contributor Author

Swapping the order of the two match arms:

error[E0282]: unable to infer enough type information about `T`
  --> /home/simon/projects/servo2/components/style/font_face.rs:66:13
   |
66 |             Ok(_) => {}
   |             ^^^^^ cannot infer type for `T`
   |
   = note: type annotations or generic parameter binding required

error: aborting due to previous error

Changing the Ok(_) pattern to Ok(()):

error[E0282]: unable to infer enough type information about `E`
  --> /home/simon/projects/servo2/components/style/font_face.rs:66:13
   |
66 |             Ok(()) => {}
   |             ^^^^^^ cannot infer type for `E`
   |
   = note: type annotations or generic parameter binding required

error: aborting due to previous error

Hacking ./mach build-geckolib to not use the stable version of the compiler (so effectively disabling the servo feature and enabling the gecko one) also gives the same error.

@nikomatsakis nikomatsakis added regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. A-typesystem Area: The type system I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 22, 2016
@SimonSapin
Copy link
Contributor Author

SimonSapin commented Dec 22, 2016

New range, based on @eddyb-guided manual bisect: 58d58c2 good, 551cb06 bad. Diff: 58d58c2...551cb06, which leaves only #38171 in-between. CC @jseyfried, @nrc.

Also found more clues. More context for the code where the error happens:

use computed_values::font_family::FontFamily;

pub struct FontFaceRule {
    pub family: FontFamily,
    // ...
}

impl ToCss for FontFaceRule {
    fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
        // ...
        try!(self.family.to_css(dest));
        // ..
    }
}

computed is a module generated by a macro, that contains pub use re-exports of other modules:

macro_rules! reexport_computed_values {
    ( $( $name: ident )+ ) => {
        pub mod computed_values {
            $(
                pub use properties::longhands::$name::computed_value as $name;
            )+
        }
    }
}

// Invoke reexport_computed_values! with a list of idents generated by Python code
longhand_properties_idents!(reexport_computed_values); 

If I add a similar pub use not in a macro:

pub mod computed_values_no_macro {
    pub use properties::longhands::font_family::computed_value as font_family;
}

or if I change the original code to use the real path without going through a pub use, this seems to work around this issue completely.

… only to uncover another one:

error: internal compiler error: cat_expr Errd
     --> /home/simon/projects/servo/target/debug/build/style-9107aeac634bb1f9/out/properties.rs:75381:40
      |
75381 |                 align_items::center => align_self::center,
      |                                        ^^^^^^^^^^^^^^^^^^

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

note: run with `RUST_BACKTRACE=1` for a backtrace

thread 'rustc' panicked at 'Box<Any>', /home/simon/projects/rust/src/librustc_errors/lib.rs:382
stack backtrace:
   1:     0x7f2b65e50e53 - std::sys::imp::backtrace::tracing::imp::write::h34c2fe4536cff926
   2:     0x7f2b65e5f63d - std::panicking::default_hook::{{closure}}::he19e50a2201d99c4
   3:     0x7f2b65e5f1db - std::panicking::default_hook::h954c9f5a176f6e88
   4:     0x7f2b65e5fac8 - std::panicking::rust_panic_with_hook::h12277e75a2b70114
   5:     0x7f2b5f5f46b7 - std::panicking::begin_panic::hc146ae5a0122e959
   6:     0x7f2b5f609d9a - rustc_errors::Handler::abort_if_errors::h714fd9b9b0557ee7
   7:     0x7f2b645c6f1f - rustc_passes::consts::check_crate::h2923cde18866be7c
   8:     0x7f2b661f4e49 - rustc_driver::driver::phase_3_run_analysis_passes::{{closure}}::h1fd32502995d307a
   9:     0x7f2b661f163e - rustc_driver::driver::phase_3_run_analysis_passes::hfd6790d5cb183751
  10:     0x7f2b661d6091 - rustc_driver::driver::compile_input::h0e69e9ade6aaa4c7
  11:     0x7f2b6621f441 - rustc_driver::run_compiler::h72646ffc571b7660
  12:     0x7f2b6613454b - std::panicking::try::do_call::hbf38fb785c7d412c
  13:     0x7f2b65e68b66 - __rust_maybe_catch_panic
  14:     0x7f2b66155aa1 - <F as alloc::boxed::FnBox<A>>::call_box::h55b6e5fdb729b74d
  15:     0x7f2b65e5e4d0 - std::sys::imp::thread::Thread::new::thread_start::h0578282da79025df
  16:     0x7f2b5ed9e453 - start_thread
  17:     0x7f2b65b257de - __GI___clone
  18:                0x0 - <unknown>

@SimonSapin
Copy link
Contributor Author

Found another ICE, I don’t know if it’s related: #38556.

@SimonSapin
Copy link
Contributor Author

Found a smaller test case for the inference issue:

fn main() {}

pub mod a {
    pub struct FontFamily;

    impl FontFamily {
        pub fn to_css(&self) -> ::std::fmt::Result {
            Ok(())
        }
    }
}

macro_rules! reexport {
    () => {
        mod b {
            pub use a;
        }
    }
}

reexport!();

pub mod c {
    use b::a::FontFamily;

    struct FontFace {
        family: FontFamily
    }

    impl FontFace {
        pub fn to_css(&self) -> ::std::fmt::Result {
            try!(self.family.to_css());
            Ok(())
        }
    }
}

@eddyb
Copy link
Member

eddyb commented Dec 22, 2016

@SimonSapin That "other ICE" is actually a direct observation of the bug, whereas this inference issue is some weird side-effect of the way we handle errors that don't stop compilation.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Dec 22, 2016

@SimonSapin when I try that test on nightly, it works...?

never mind, apparently it fails locally

@nikomatsakis
Copy link
Contributor

triage: P-high

Seems bad. Assigning to @jseyfried based on bisection.

@arielb1
Copy link
Contributor

arielb1 commented Dec 22, 2016

Does this affect beta (3f9823d)?

@SimonSapin
Copy link
Contributor Author

@nikomatsakis At the moment the latest successful nightly build is 3 days old. I don’t know how often playpen’s nigthly is updated. #38499 merged 2 days ago. (This is the rollup PR that merged #38171, which introduced this issue if I did my manual bisecting correctly.)

@arielb1 As far as I can tell no, 551cb06 is not reachable from 3f9823d and so beta is not affected.

@jseyfried
Copy link
Contributor

Fixed in #38566.

bors added a commit that referenced this issue Dec 25, 2016
Fix bug in import resolution

Fixes #38535 and fixes #38556.
r? @nrc
bors-servo pushed a commit to servo/servo that referenced this issue Jan 2, 2017
Update to rustc 1.16.0-nightly (4ecc85beb 2016-12-28)

<s>**This is not ready to land** since there is no corresponding Nightly build of Rust yet.</s> Update: we got a Nightly build on 2016-12-29: http://rusty-dash.com/nightlies

I made these changes to check that rust-lang/rust#38566 fixes rust-lang/rust#38535 (which it does, yay!) so I might as well publish them, we’ll need them soon enough.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14689)
<!-- Reviewable:end -->
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Feb 4, 2017
…12-28) (from servo:rustup); r=Manishearth

<s>**This is not ready to land** since there is no corresponding Nightly build of Rust yet.</s> Update: we got a Nightly build on 2016-12-29: http://rusty-dash.com/nightlies

I made these changes to check that rust-lang/rust#38566 fixes rust-lang/rust#38535 (which it does, yay!) so I might as well publish them, we’ll need them soon enough.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 9bdd0f401a89398fb2ecd4f6b54691a7c93b2e53
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 1, 2019
…12-28) (from servo:rustup); r=Manishearth

<s>**This is not ready to land** since there is no corresponding Nightly build of Rust yet.</s> Update: we got a Nightly build on 2016-12-29: http://rusty-dash.com/nightlies

I made these changes to check that rust-lang/rust#38566 fixes rust-lang/rust#38535 (which it does, yay!) so I might as well publish them, we’ll need them soon enough.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 9bdd0f401a89398fb2ecd4f6b54691a7c93b2e53

UltraBlame original commit: 17e25ccaa7dfe2f7c7d88878a1fb5ce5f1888914
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 1, 2019
…12-28) (from servo:rustup); r=Manishearth

<s>**This is not ready to land** since there is no corresponding Nightly build of Rust yet.</s> Update: we got a Nightly build on 2016-12-29: http://rusty-dash.com/nightlies

I made these changes to check that rust-lang/rust#38566 fixes rust-lang/rust#38535 (which it does, yay!) so I might as well publish them, we’ll need them soon enough.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 9bdd0f401a89398fb2ecd4f6b54691a7c93b2e53

UltraBlame original commit: 17e25ccaa7dfe2f7c7d88878a1fb5ce5f1888914
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 1, 2019
…12-28) (from servo:rustup); r=Manishearth

<s>**This is not ready to land** since there is no corresponding Nightly build of Rust yet.</s> Update: we got a Nightly build on 2016-12-29: http://rusty-dash.com/nightlies

I made these changes to check that rust-lang/rust#38566 fixes rust-lang/rust#38535 (which it does, yay!) so I might as well publish them, we’ll need them soon enough.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 9bdd0f401a89398fb2ecd4f6b54691a7c93b2e53

UltraBlame original commit: 17e25ccaa7dfe2f7c7d88878a1fb5ce5f1888914
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-typesystem Area: The type system P-high High priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
6 participants