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

NLL: change compare-mode=nll to use borrowck=migrate #55134

Merged
merged 3 commits into from Oct 18, 2018

Conversation

Projects
None yet
5 participants
@davidtwco
Member

davidtwco commented Oct 16, 2018

Fixes #55118.

This PR is split into two parts:

The first commit is a minor change that fixes a flaw in the existing borrowck=migrate implementation whereby a lint that was promoted to an error in the AST borrow checker would result in the same lint from the NLL borrow checker being downgraded to a warning in migrate mode. This PR fixes this by ensuring lints are exempt from buffering in the NLL borrow checker.

The second commit updates compiletest to make the NLL compare mode use -Z borrowck=migrate rather than -Z borrowck=mir. The third commit shows all the test output changes that result from this.

r? @pnkfelix

davidtwco added some commits Oct 16, 2018

Don't buffer lints.
When lints are emitted from the AST borrow checker, they do not signal
an error as it is not known at that time whether, due to attributes,
that lint will error or warn. This means that when lints are buffered
in the MIR they will always be downgraded, as the AST borrowck will not
have been marked as having errored, even if a lint was upgraded to
an error after being emitted from the AST borrowck. The simple solution
to this is to not buffer any lints from the MIR borrowck.
Change NLL compare mode to borrowck=migrate.
This commit changes the NLL compare mode to pass `-Z borrowck=migrate`
rather than `-Z borrowck=nll` to better test what will be deployed. It
does not include the test output updates, as separation of these commits
makes reviewing simpler.
Update output for borrowck=migrate compare mode.
This commit updates the test output for the updated NLL compare mode
that uses `-Z borrowck=migrate` rather than `-Z borrowck=mir`. The
previous commit changes `compiletest` and this commit only updates
`.nll.stderr` files.
@davidtwco

This comment has been minimized.

Member

davidtwco commented Oct 16, 2018

At request of @pnkfelix, the first commit has a separate PR (#55135) so that if this PR needs a lot of rebases, it doesn't slow that fix down.

@pnkfelix

This comment has been minimized.

Member

pnkfelix commented Oct 17, 2018

This PR looks fine as it stands. I didn't read over everything in commit 539404b, but I did some spot checking of the changes in output.

However, there is some debate about whether this PR should land before or after we do the third and final round of .stderr review (issue #54528) ... so I'm delaying slightly on r+'ing this until after we've resolved that Question.

@pnkfelix

This comment has been minimized.

Member

pnkfelix commented Oct 17, 2018

From @nikomatsakis:

The options:

  • land the change to migrate mode and do a comparison of migrate/ast output
  • do a comparison of nll/ast output and then land change to migrate mode?

I see pros/cons to both, but it seems to me that, schedule wise, it is most urgent to ensure migration "seems good", and we can revisit NLL "at our leisure".

To that end, there is a third-way add the NLL compare mode but do not run it on the builders. You can then run it manually to generate the comparison files for the purposes of comparison.

That makes sense to me. So I say, we land this now. And we can figure out what to do for #54528 after it lands.

@pnkfelix

This comment has been minimized.

Member

pnkfelix commented Oct 17, 2018

@bors r+ p=1

@bors

This comment has been minimized.

Contributor

bors commented Oct 17, 2018

📌 Commit 539404b has been approved by pnkfelix

@pnkfelix

This comment has been minimized.

Member

pnkfelix commented Oct 17, 2018

(i gave this priority=1 because I'm concerned about the .stderr files going stale quickly.)

@bors

This comment has been minimized.

Contributor

bors commented Oct 17, 2018

⌛️ Testing commit 539404b with merge 4e9f799...

bors added a commit that referenced this pull request Oct 17, 2018

Auto merge of #55134 - davidtwco:issue-55118, r=pnkfelix
NLL: change compare-mode=nll to use borrowck=migrate

Fixes #55118.

This PR is split into two parts:

The first commit is a minor change that fixes a flaw in the existing `borrowck=migrate` implementation whereby a lint that was promoted to an error in the AST borrow checker would result in the same lint from the NLL borrow checker being downgraded to a warning in migrate mode. This PR fixes this by ensuring lints are exempt from buffering in the NLL borrow checker.

The second commit updates `compiletest` to make the NLL compare mode use `-Z borrowck=migrate` rather than `-Z borrowck=mir`. The third commit shows all the test output changes that result from this.

r? @pnkfelix
@bors

This comment has been minimized.

Contributor

bors commented Oct 17, 2018

💔 Test failed - status-travis

@davidtwco

This comment has been minimized.

Member

davidtwco commented Oct 17, 2018

@bors retry

@bors

This comment has been minimized.

Contributor

bors commented Oct 17, 2018

⌛️ Testing commit 539404b with merge 43ab5b1...

bors added a commit that referenced this pull request Oct 17, 2018

Auto merge of #55134 - davidtwco:issue-55118, r=pnkfelix
NLL: change compare-mode=nll to use borrowck=migrate

Fixes #55118.

This PR is split into two parts:

The first commit is a minor change that fixes a flaw in the existing `borrowck=migrate` implementation whereby a lint that was promoted to an error in the AST borrow checker would result in the same lint from the NLL borrow checker being downgraded to a warning in migrate mode. This PR fixes this by ensuring lints are exempt from buffering in the NLL borrow checker.

The second commit updates `compiletest` to make the NLL compare mode use `-Z borrowck=migrate` rather than `-Z borrowck=mir`. The third commit shows all the test output changes that result from this.

r? @pnkfelix
@bors

This comment has been minimized.

Contributor

bors commented Oct 17, 2018

💔 Test failed - status-travis

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Oct 17, 2018

The job dist-i586-gnu-i586-i686-musl of your PR failed on Travis (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.
travis_fold:end:dpl_0
travis_time:start:01bdebe5
travis_fold:start:dpl.1
Installing deploy dependencies
ERROR:  Could not find a valid gem 'aws-sdk-resources' (= 2.11.152) in any repository
ERROR:  Possible alternatives: aws-sdk-resources
travis_fold:end:dpl.1
/home/travis/.rvm/rubies/ruby-2.2.7/lib/ruby/site_ruby/2.2.0/rubygems/core_ext/kernel_require.rb:59:in `require': cannot load such file -- dpl/provider/s3 (LoadError)
 from /home/travis/.rvm/rubies/ruby-2.2.7/lib/ruby/site_ruby/2.2.0/rubygems/core_ext/kernel_require.rb:59:in `require'
 from /home/travis/.rvm/gems/ruby-2.2.7/gems/dpl-1.10.3/lib/dpl/provider.rb:94:in `rescue in block in new'
 from /home/travis/.rvm/gems/ruby-2.2.7/gems/dpl-1.10.3/lib/dpl/provider.rb:69:in `block in new'
 from /home/travis/.rvm/gems/ruby-2.2.7/gems/dpl-1.10.3/lib/dpl/cli.rb:41:in `fold'
 from /home/travis/.rvm/gems/ruby-2.2.7/gems/dpl-1.10.3/lib/dpl/provider.rb:68:in `new'
 from /home/travis/.rvm/gems/ruby-2.2.7/gems/dpl-1.10.3/lib/dpl/cli.rb:31:in `run'
 from /home/travis/.rvm/gems/ruby-2.2.7/gems/dpl-1.10.3/lib/dpl/cli.rb:7:in `run'
 from /home/travis/.rvm/gems/ruby-2.2.7/gems/dpl-1.10.3/bin/dpl:5:in `<top (required)>'
 from /home/travis/.rvm/gems/ruby-2.2.7/bin/dpl:23:in `load'
 from /home/travis/.rvm/gems/ruby-2.2.7/bin/dpl:23:in `<main>'
failed to deploy

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 @TimNN. (Feature Requests)

@Mark-Simulacrum

This comment has been minimized.

Member

Mark-Simulacrum commented Oct 17, 2018

@bors retry

Should definitely not be related to this...

@bors

This comment has been minimized.

Contributor

bors commented Oct 17, 2018

⌛️ Testing commit 539404b with merge f7eb7fb...

bors added a commit that referenced this pull request Oct 17, 2018

Auto merge of #55134 - davidtwco:issue-55118, r=pnkfelix
NLL: change compare-mode=nll to use borrowck=migrate

Fixes #55118.

This PR is split into two parts:

The first commit is a minor change that fixes a flaw in the existing `borrowck=migrate` implementation whereby a lint that was promoted to an error in the AST borrow checker would result in the same lint from the NLL borrow checker being downgraded to a warning in migrate mode. This PR fixes this by ensuring lints are exempt from buffering in the NLL borrow checker.

The second commit updates `compiletest` to make the NLL compare mode use `-Z borrowck=migrate` rather than `-Z borrowck=mir`. The third commit shows all the test output changes that result from this.

r? @pnkfelix
@bors

This comment has been minimized.

Contributor

bors commented Oct 18, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: pnkfelix
Pushing f7eb7fb to master...

@bors bors merged commit 539404b into rust-lang:master Oct 18, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@davidtwco davidtwco deleted the davidtwco:issue-55118 branch Oct 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment