Make privacy checking aware that a use directive can point to two defintions (namespaces) with different privacy.

[nrc]

closes #4110

[alexcrichton]

It seems a little odd to see PrivMod(AllPublic), so perhaps LastMod(AllPublic) and LastImport?

[alexcrichton]

Marvelous tests, amazing design, nice job!

The only major comment I have is that if you use Bar; then you get the error message at the location of use Bar;, not the usage of the private Bar. This seems a little odd because the error message isn't quite accurate. The error message could in theory say whether it's private in which namespace, but I don't think that'd be too useful either because most shouldn't have to know about the two namespaces.

I think that we definitely want to land this at least as-is because it fixes the backwards-compatibility issues. Error messages can always be refined later down the road. I think something like this could be fixed with a side table mapping paths to the imports they were resolved with, but at some point this is also diminishing returns because this is a fairly rare use case anyway.

So, two things:

1. What do you think about reporting the error at the path vs the import?
2. If we leave as-is, could you tag the location with a FIXME and open an issue about it?

Again, awesome work!

[nrc]

The reason the error messages are a bit wrong is that I didn't want the def map to point at two things for imports - I thought that would be too irregular. But that means we don't really know if we are pointing at a type or a value in privacy.rs and so we sometimes get a slightly wrong error message. In other words, although the privacy information is aware of namespaces, the definition information is not. So when we rely on the latter we get incomplete/incorrect info. This only happens for error messages, so I didn't think it was breaking a major assumption about definitions for.

1. I prefer to leave the error message on the import rather than the use of the import because the actual error is with the import - it is the import referring to something which is private, not the path - which is referring to the import which is public.
2. So, I think the thing to fix is the error messages - I can certainly do those things if you agree this is what should be fixed.

[alexcrichton]

I like your line of reasoning for putting the error message on the import. That doesn't have to be done as part of this PR, however, feel free to open an issue and work on it later.

[flaper87]

I like this report method and I think it could be used in other places where the same form of reporting is necessary. What do you think about making this a default method in ast::visit::Visitor (or moving it somewhere else)? If you prefer to respect YAGNI, I can do it myself later, I've already a case where I'd benefit from this.

[flaper87]

Actually, maybe this could go in the session itself.

[nrc]

Yeah, I was thinking about maybe pulling out CheckResult and report_error into somewhere more generic - I was thinking of some utils file somewhere. The session is probably a good place (I prefer Visitor since it is not really related to visiting). I decided not to for now since I didn't know if it was generally useful. If you think it would be, feel free to pull it out.

[flaper87]

I think it could be useful and I could use it in the patch I'm currently working on. Also, the pattern Error + Note is used in other places too.

I can pull it out later. This is not a blocker for this patch at all.

[flaper87]

Another case where this could be helpful #12347

[nrc]

@alexcrichton also ready for re-review

[alexcrichton]

Feel free to // ignore-fast the failing test, it's not compatible with the check-fast suite anyway

[nrc]

@bors retry

[huonw]

Was this revert ever fixed?

[alexcrichton]

Hm oddly enough 800b56fe6af21ffd8e56aee8cf12dd758f5bbdf1 is the right revision, not sure what's going on here...