Skip to content

Change error contains interior mutability to may contain interior mutability? #40313

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.

Sign up for GitHub

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

Jump to bottom
Closed
dpc opened this issue Mar 7, 2017 · 13 comments
Closed

Change error contains interior mutability to may contain interior mutability? #40313

dpc opened this issue Mar 7, 2017 · 13 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.

Comments

@dpc
Copy link
Contributor

dpc commented Mar 7, 2017

The following error is a bit misleading:

the type SomeTrait + 'static contains interior mutability and a reference may not be safely transferrable across a catch_unwind boundary

Since SomeTrait is a trait object, it seems to me may contain (...) is more precise.
@Mark-Simulacrum Mark-Simulacrum added the A-diagnostics Area: Messages for errors, warnings, and lints label May 27, 2017
@Mark-Simulacrum
Copy link
Member

@rust-lang/lang: Do we want to make this change? If so, E-easy and E-mentor on me. I think it makes sense to edit the language; unfortunately I don't think we can do it only when referencing a trait object.

This message needs to be updated to the new language and src/test/compile-fail/issue-17718-const-borrow.rs and src/librustc_mir/diagnostics.rs need updating as of the time of writing this.

@nikomatsakis
Copy link
Contributor

may contain seems fine. We are perhaps a bit inconsistent on this -- it often happens that we make static approximations, and hence all the compiler's conclusions are sort of (implicitly) "may contain foo". Seems like it's probably better to try to be explicit about that in our phrasing, though, since it can really throw people otherwise.

@Mark-Simulacrum Mark-Simulacrum added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels May 30, 2017
@Mark-Simulacrum
Copy link
Member

Okay, I've marked as E-easy and E-mentor; initial instructions are here. Let us know if you'd like to work on this or have any questions, we'd be happy to help.

@gaurikholkar-zz
Copy link

I'd like to take this up

@Mark-Simulacrum
Copy link
Member

Go ahead! Let us know if you'd like any assistance.

@gaurikholkar-zz
Copy link

So we aren't doing it only while referencing a TraitObject right?

@Mark-Simulacrum
Copy link
Member

No, since that'd require doing this inside the compiler; we can get 95% of the way there by just changing the error message. There's not a good way to check in the annotation if the Self type is a trait object.

@gaurikholkar-zz
Copy link

gaurikholkar-zz commented Jun 1, 2017 •
edited
Loading

Here's a link to my local branch. I've made the changes as you suggested followed by a ./x.py build --stage 0 and ./x.py build --stage 1. But the compile-fail test seems to be failing. The change does not seem to get reflected somehow. Do I need to do something more?

@Mark-Simulacrum
Copy link
Member

Could you post a build log? By the way, I think to test you should run ./x.py test --incremental src/test/compile-fail in this case and that should be good.

@eddyb
Copy link
Member

eddyb commented Jun 1, 2017

You changed RefUnwindSafe's on_unimplemented message, which is relevant to this issue, but none of the testcases you changed actually use RefUnwindSafe (they just happen to have similar phrasing, but they're about borrowing values with UnsafeCell in them, in constants).

@gaurikholkar-zz
Copy link

@eddyb thanks! I happened to not notice that these two were indeed different. The error message regarding borrowing values appears here . That'll make the compile-fail test work. I'll write a new ui test for the RefUnwindSafe case as discussed with @nikomatsakis .

@gaurikholkar-zz
Copy link

gaurikholkar-zz commented Jun 6, 2017 •
edited
Loading

Updated the code here
./x.py test src/test/compile-fail works
./x.py test src/test/ui works
Should I open up a PR?

@eddyb
Copy link
Member

eddyb commented Jun 6, 2017

@gaurikholkar Yupp, looks great!

@gaurikholkar-zz gaurikholkar-zz mentioned this issue Jun 6, 2017
Merged
frewsxcv added a commit to frewsxcv/rust that referenced this issue Jun 8, 2017
@frewsxcv
Rollup merge of rust-lang#42490 - gaurikholkar:master, r=eddyb
3ce88c7 
Changing error message from `contains interior mutability` to `may contain interior mutability`

Fixes rust-lang#40313 . I have changed the message from `contains interior mutability` to `may contain interior mutability` for the following example
```
use std::cell::Cell;
use std::panic::catch_unwind;
fn main() {
    let mut x = Cell::new(22);
    catch_unwind(|| { x.set(23); });
}
```
which has been added as a ui test.

Also, the message [here](https://github.com/gaurikholkar/rust/blob/master/src/librustc_mir/transform/qualify_consts.rs#L666) and it's respective `compile-fail` test have been modified.

cc @nikomatsakis  @Mark-Simulacrum  @eddyb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-diagnostics Area: Messages for errors, warnings, and lints E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@dpc @eddyb @nikomatsakis @Mark-Simulacrum @gaurikholkar-zz