Generic arg disambiguation #66104

yodaldevoid · 2019-11-05T05:19:22Z

Using the tactic suggested by @petrochenkov in #60804 (comment) and on zulip, this change checks type arguments to see if they are really incorrectly-parsed const arguments.

it should be noted that segments.len() == 1 && segments[0].arg.is_none() was reduced to segments.len() == 1 as suggested by @petrochenkov in zulip. This change allowed a few more existing tests to have their braces removed.

There are a couple of "problems" with these changes that I should note. First, there was a regression in the error messages found in "src/test/ui/privacy-ns1.rs" and "src/test/ui/privacy-ns1.rs". Second, some braces were unable to be removed from "src/test/ui/const-generics/fn-const-param-infer.rs". Those on line 24 caused the statement to stop equating when removed, and those on line 20 cause a statement that should not equate to produce no error when removed.

I have not looked further into any of these issues yet, though I would be willing to look into them before landing this. I simply wanted to get some other eyes on this before going further.

Fixes #60804

cc @varkor @jplatte

rust-highfive · 2019-11-05T05:19:25Z

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

src/librustc/hir/lowering.rs

src/librustc_resolve/late.rs

src/librustc/hir/lowering.rs

src/test/ui/const-generics/const-generic-array-wrapper.rs

src/test/ui/const-generics/fn-const-param-call.rs

eddyb

Mostly LGTM, I'm only worried about the "value namespace" check, maybe @petrochenkov has some ideas there?

petrochenkov · 2019-11-07T19:33:01Z

This will probably have to wait until the weekend.
r? @petrochenkov

src/librustc/hir/lowering.rs

src/librustc/hir/def.rs

src/librustc/hir/lowering.rs

src/librustc_resolve/late.rs

src/test/ui/const-generics/fn-const-param-infer.stderr

varkor · 2019-11-08T15:34:41Z

src/test/ui/privacy/privacy-ns1.stderr

@@ -86,7 +64,19 @@ LL | use foo2::Bar;
 LL | use foo3::Bar;
   |

-error: aborting due to 4 previous errors
+error[E0107]: wrong number of const arguments: expected 0, found 1


This is interesting fallout. I imagine we should be able to special-case this sort of error to get back the old ones, though I'm not sure how much work that would be.

As far as I remember, the fix may be non-trivial and error-prone, so it's better to make an issue and fix it in a separate PR.

src/librustc/hir/def.rs

src/librustc/hir/lowering.rs

petrochenkov · 2019-11-09T16:12:30Z

src/librustc/hir/lowering.rs

+                                        DefPathData::AnonConst,
+                                        ExpnId::root(),
+                                        ty.span,
+                                    );


Hmm, this distinction between types and consts (consts requiring an extra def path node) is something I didn't expect.
@eddyb, is it possible that creating this node during lowering is too late?

Maybe? But if you limit this to a really simple path with no generic args, there shouldn't be any children defs nested inside so making a leaf AnonConst should be fine.

@yodaldevoid
Could you bring back the "no generic args" restriction then?
(It should rarely be relevant in practice.)

It is done.

It should be noted that this does mean that we cannot remove all of the braces from the const-generics tests, specifically in "fn-const-param-infer.rs" which uses type params all over the place. How hard/wise would it be to iterate through the children of the new def to create defs for them if needed?

How hard/wise would it be to iterate through the children of the new def to create defs for them if needed?

I'm not sure.
First, we need to at least construct a test where things would break due to def paths not being constructed properly.
This is a material for another PR, IMO.
We get, like, 90% of benefits from disambiguating simple identifiers, so it would be nice to land this PR sooner.

I think we already have such a test in "fn-const-param-infer.rs", but I agree that is something for a future PR.

src/librustc/hir/lowering.rs

src/librustc_resolve/late.rs

petrochenkov · 2019-11-09T16:29:23Z

src/test/ui/privacy/privacy-ns1.stderr

@@ -86,7 +64,19 @@ LL | use foo2::Bar;
 LL | use foo3::Bar;
   |

-error: aborting due to 4 previous errors
+error[E0107]: wrong number of const arguments: expected 0, found 1


As far as I remember, the fix may be non-trivial and error-prone, so it's better to make an issue and fix it in a separate PR.

yodaldevoid · 2019-11-09T21:00:42Z

I believe I have addressed all comments given so far. There is one comment that I would like to have resolved before we move forward with these changes.

petrochenkov · 2019-11-10T09:32:20Z

@yodaldevoid

What exactly is the reasoning behind only allowing single segment consts here? I'm not sure I've ever seen the reason spelled out.

We need to disambiguate in favor of types, but for multi-segment paths we cannot do that until type checking in general case (due to associated types).
https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/generic.20argument.20disambiguation/near/177847619

We can do some "figure disambiguation" for multi-segment paths to accept some cases, but that complicates the rules, and other alternatives like delaying the disambiguation until the generic argument substitution is really required (in type checking) may be preferable.

src/librustc/hir/lowering.rs

src/librustc_resolve/late.rs

Braces were left in cases where generic args were in the generic const paths.

The error messages of the two tests effected degraded in quality. The errors no longer suggest types in other modules as they now assume that the arguments are const args, not type args.

petrochenkov · 2019-11-19T18:15:06Z

@bors r+

bors · 2019-11-19T18:15:07Z

📌 Commit 0207a15 has been approved by petrochenkov

bors · 2019-11-19T18:37:27Z

⌛ Testing commit 0207a15 with merge 0a431b8e96bfc211dd2247e3cd56105e589b13a2...

rust-highfive · 2019-11-19T18:42:34Z

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.

2019-11-19T18:41:44.6383257Z Attempting with retry: docker build --rm -t rust-ci -f /home/vsts/work/1/s/src/ci/docker/dist-x86_64-netbsd/Dockerfile /home/vsts/work/1/s/src/ci/docker
2019-11-19T18:41:44.8247314Z Sending build context to Docker daemon  528.4kB
2019-11-19T18:41:44.8247599Z 
2019-11-19T18:41:44.8847651Z Step 1/12 : FROM ubuntu:16.04
2019-11-19T18:41:45.5615847Z Get https://registry-1.docker.io/v2/library/ubuntu/manifests/16.04: received unexpected HTTP status: 500 Internal Server Error
2019-11-19T18:41:46.7933149Z Sending build context to Docker daemon  528.4kB
2019-11-19T18:41:46.7933379Z 
2019-11-19T18:41:46.8166438Z Step 1/12 : FROM ubuntu:16.04
2019-11-19T18:41:46.8166438Z Step 1/12 : FROM ubuntu:16.04
2019-11-19T18:41:47.3936219Z Get https://registry-1.docker.io/v2/library/ubuntu/manifests/16.04: received unexpected HTTP status: 500 Internal Server Error
2019-11-19T18:41:49.5810512Z Sending build context to Docker daemon  528.4kB
2019-11-19T18:41:49.5810778Z 
2019-11-19T18:41:49.6004519Z Step 1/12 : FROM ubuntu:16.04
2019-11-19T18:41:49.6004519Z Step 1/12 : FROM ubuntu:16.04
2019-11-19T18:41:50.0978698Z Get https://registry-1.docker.io/v2/library/ubuntu/manifests/16.04: received unexpected HTTP status: 500 Internal Server Error
2019-11-19T18:41:53.2482214Z Sending build context to Docker daemon  528.4kB
2019-11-19T18:41:53.2487663Z 
2019-11-19T18:41:53.2766217Z Step 1/12 : FROM ubuntu:16.04
2019-11-19T18:41:53.2766217Z Step 1/12 : FROM ubuntu:16.04
2019-11-19T18:41:53.7856733Z Get https://registry-1.docker.io/v2/library/ubuntu/manifests/16.04: received unexpected HTTP status: 500 Internal Server Error
2019-11-19T18:41:57.9035917Z Sending build context to Docker daemon  528.4kB
2019-11-19T18:41:57.9036879Z 
2019-11-19T18:41:57.9245669Z Step 1/12 : FROM ubuntu:16.04
2019-11-19T18:41:57.9245669Z Step 1/12 : FROM ubuntu:16.04
2019-11-19T18:41:58.4398790Z Get https://registry-1.docker.io/v2/library/ubuntu/manifests/16.04: received unexpected HTTP status: 500 Internal Server Error
2019-11-19T18:41:58.4470573Z 
2019-11-19T18:41:58.4470573Z 
2019-11-19T18:41:58.4581131Z ##[error]Bash exited with code '1'.
2019-11-19T18:41:58.4614098Z ##[section]Starting: Checkout
2019-11-19T18:41:58.4616398Z ==============================================================================
2019-11-19T18:41:58.4616517Z Task         : Get sources
2019-11-19T18:41:58.4616601Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

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)

bors · 2019-11-19T18:42:39Z

💔 Test failed - checks-azure

varkor · 2019-11-19T22:33:49Z

@bors retry

bors · 2019-11-20T03:07:46Z

⌛ Testing commit 0207a15 with merge f50d6ea...

@petrochenkov

…ochenkov Generic arg disambiguation Using the tactic suggested by @petrochenkov in #60804 (comment) and on [zulip](https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/generic.20argument.20disambiguation), this change checks type arguments to see if they are really incorrectly-parsed const arguments. it should be noted that `segments.len() == 1 && segments[0].arg.is_none()` was reduced to `segments.len() == 1` as suggested by @petrochenkov in [zulip](https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/generic.20argument.20disambiguation/near/177848002). This change allowed a few more existing tests to have their braces removed. There are a couple of "problems" with these changes that I should note. First, there was a regression in the error messages found in "src/test/ui/privacy-ns1.rs" and "src/test/ui/privacy-ns1.rs". Second, some braces were unable to be removed from "src/test/ui/const-generics/fn-const-param-infer.rs". Those on line 24 caused the statement to stop equating when removed, and those on line 20 cause a statement that should not equate to produce no error when removed. I have not looked further into any of these issues yet, though I would be willing to look into them before landing this. I simply wanted to get some other eyes on this before going further. Fixes #60804 cc @varkor @jplatte

bors · 2019-11-20T06:34:00Z

☀️ Test successful - checks-azure
Approved by: petrochenkov
Pushing f50d6ea to master...

rust-highfive · 2019-11-20T06:35:19Z

📣 Toolstate changed by #66104!

Tested on commit f50d6ea.
Direct link to PR: #66104

🎉 rustc-guide on linux: test-fail → test-pass (cc @JohnTitor @amanjeev @spastorino @mark-i-m, @rust-lang/infra).

@JohnTitor

Tested on commit rust-lang/rust@f50d6ea. Direct link to PR: <rust-lang/rust#66104> 🎉 rustc-guide on linux: test-fail → test-pass (cc @JohnTitor @amanjeev @spastorino @mark-i-m, @rust-lang/infra).

rust-highfive assigned eddyb Nov 5, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 5, 2019

petrochenkov self-assigned this Nov 5, 2019

jplatte reviewed Nov 5, 2019

View reviewed changes

src/librustc/hir/lowering.rs Outdated Show resolved Hide resolved

src/librustc_resolve/late.rs Outdated Show resolved Hide resolved

varkor added the F-const_generics `#![feature(const_generics)]` label Nov 5, 2019

eddyb reviewed Nov 6, 2019

View reviewed changes

src/librustc/hir/lowering.rs Outdated Show resolved Hide resolved

eddyb reviewed Nov 6, 2019

View reviewed changes

src/test/ui/const-generics/const-generic-array-wrapper.rs Show resolved Hide resolved

eddyb reviewed Nov 6, 2019

View reviewed changes

src/test/ui/const-generics/fn-const-param-call.rs Show resolved Hide resolved

eddyb approved these changes Nov 6, 2019

View reviewed changes

rust-highfive unassigned eddyb Nov 7, 2019