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

Move effect-checking to MIR #44700

Merged
merged 5 commits into from
Sep 25, 2017
Merged

Move effect-checking to MIR #44700

merged 5 commits into from
Sep 25, 2017

Conversation

arielb1
Copy link
Contributor

@arielb1 arielb1 commented Sep 19, 2017

This allows emitting lints from MIR and moves the effect-checking pass to work on it.

I'll make repr(packed) misuse unsafe in a separate PR.

r? @eddyb

@eddyb
Copy link
Member

eddyb commented Sep 19, 2017

LGTM. Unsure about having a separate vector for visibility scope information (and I'd rename them to "lexical" or "source" scopes). r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis and unassigned eddyb Sep 19, 2017

impl<T> serialize::Decodable for ClearOnDecode<T> {
fn decode<D: serialize::Decoder>(d: &mut D) -> Result<Self, D::Error> {
serialize::Decodable::decode(d).map(|_v: ()| ClearOnDecode::Clear)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.map(|()| ...) is possible here. (() matches the only val of type ()

@@ -75,6 +76,22 @@ for mir::Terminator<'gcx> {
}
}

impl<'a, 'gcx, 'tcx, T> HashStable<StableHashingContext<'a, 'gcx, 'tcx>> for mir::ClearOnDecode<T>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StableHashingContext now only takes one lifetime: StableHashingContext<'gcx>.

@arielb1 arielb1 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 19, 2017
@bors
Copy link
Contributor

bors commented Sep 20, 2017

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

/// The *lexical* visibility scope the local is defined
/// in. If the local was defined in a let-statement, this
/// is *within* the let-statement, rather than outside
/// of iit.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(typo, "iit")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hate that keyboard. should get a new one that is actually good.

@@ -1004,15 +1004,16 @@ pub fn phase_3_run_analysis_passes<'tcx, F, R>(sess: &'tcx Session,
// FIXME: ariel points SimplifyBranches should run after
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... we can remove the FIXME as part of this PR, right?

@pnkfelix
Copy link
Member

(LGTM too.)

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty awesome. r=me but I'm not sure what's up with the THIS IS A TEST comment.

@@ -487,6 +487,16 @@ impl<'a, 'gcx, 'tcx> TypeChecker<'a, 'gcx, 'tcx> {

if self.is_box_free(func) {
self.check_box_free_inputs(mir, term, &sig, args);
// THIS IS A TEST. TEST TEST.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does this comment mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to see that lints worked before I actually used them to implement effect checking.

Copy link
Contributor Author

@arielb1 arielb1 Sep 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops. this should never have reached github. removing.

@arielb1 arielb1 force-pushed the mir-effectck branch 2 times, most recently from f4b3b24 to 48466b7 Compare September 20, 2017 21:29
@arielb1
Copy link
Contributor Author

arielb1 commented Sep 20, 2017

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Sep 20, 2017

📌 Commit 48466b7 has been approved by nikomatsakis

@nikomatsakis nikomatsakis 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 Sep 21, 2017
@bors
Copy link
Contributor

bors commented Sep 22, 2017

⌛ Testing commit 48466b75cf16294f1d3bef913f2ac4c34cb80a94 with merge 3a3144bc996f5950bc3415dd6c7325ed87ebbd2c...

@bors
Copy link
Contributor

bors commented Sep 22, 2017

💔 Test failed - status-appveyor

@@ -16,6 +16,8 @@ type Foo = std::cell::RefCell<String>;
#[cfg(target_thread_local)]
static __KEY: std::thread::__FastLocalKeyInner<Foo> =
std::thread::__FastLocalKeyInner::new();
//~^^ ERROR Sync` is not satisfied
//~^^^ ERROR Sync` is not satisfied
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check x86_64-pc-windows-gnu failed, can't find these two errors.

Windows aren't cfg(target_thread_local) IIRC.

---- [compile-fail] compile-fail\issue-43733.rs stdout ----
	
error: C:/projects/rust/src/test/compile-fail/issue-43733.rs:17: expected error not found: Sync` is not satisfied
error: C:/projects/rust/src/test/compile-fail/issue-43733.rs:17: expected error not found: Sync` is not satisfied
error: 0 unexpected errors found, 2 expected errors not found

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only mingw windows. MSVC windows does have cfg(target_thread_local).
However... this error shouldn't be there, I think I forgot to put #[thread_local] on it when I wrote the test? Or maybe I wrote the test, then later changed #[thread_local] and forgot to update the test (because it didn't give these errors because MIR checks run far too late :/).

Anyway, just adding #[thread_local] should work.

@arielb1
Copy link
Contributor Author

arielb1 commented Sep 24, 2017

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Sep 24, 2017

📌 Commit 516534f has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Sep 25, 2017

⌛ Testing commit 516534f with merge 7a9cdc4...

bors added a commit that referenced this pull request Sep 25, 2017
Move effect-checking to MIR

This allows emitting lints from MIR and moves the effect-checking pass to work on it.

I'll make `repr(packed)` misuse unsafe in a separate PR.

r? @eddyb
@bors
Copy link
Contributor

bors commented Sep 25, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 7a9cdc4 to master...

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants