Skip to content

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Dec 16, 2019

The previous check demanded that the package prefix of a file corresponds
one-to-one with the directory name. This makes it illegal to have classes
in nested package clauses inside a file, which seems unreasonable. We now
only require that the file path is a prefix of the package path.

The previous check demanded that the package prefix of a file corresponds
one-to-one with the directory name. This makes it illegal to have classes
in nested package clauses inside a file, which seems unreasonable. We now
only require that the file path is a prefix of the package path.
Copy link
Contributor

@nicolasstucki nicolasstucki left a comment

Choose a reason for hiding this comment

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

The issue number on the commit should be #7781. Otherwise LGTM.

Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

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

This also lacks a test.

@nicolasstucki
Copy link
Contributor

@smarter do you know how to create a test that triggers this warning?

@smarter
Copy link
Member

smarter commented Dec 17, 2019

Use tests/pos-special/sourcepath and tests/neg-custom-args/sourcepath

@odersky odersky changed the title Fix #7783: Loosen check for classes in wrong directory Fix #7781: Loosen check for classes in wrong directory Dec 17, 2019
@odersky
Copy link
Contributor Author

odersky commented Dec 17, 2019

Isn't this going to enter classes with the wrong owner?

I don't think so. Why would it?

This also lacks a test.

Well, in a sense the quoted.Expr example is the test. That's what failed before, anyway.

I won't be able to work on this anymore, but it needs to be part of the release. @smarter or @nicolasstucki maybe you want to add a test? If there's no time for that let's merge anyway.

@odersky odersky removed their assignment Dec 17, 2019
@smarter
Copy link
Member

smarter commented Dec 17, 2019

Isn't this going to enter classes with the wrong owner?

I don't think so. Why would it?

Because owner comes from the path and will be the same for everything entered by enterScanned. Demonstration with this PR:

// A.scala
package foo
package bar

class A
// B.scala
type Bla = foo.bar.A
% mkdir sp/foo
% mv A.scala sp/foo
% dotc -sourcepath sp B.scala
-- [E008] Member Not Found Error: B.scala:1:15 ------------------------------
1 |type Bla = foo.bar.A
  |           ^^^^^^^
  |           value bar is not a member of foo - did you mean foo.A?

No symbol was created for foo.bar, let alone foo.bar.A, and A was mistakenly added in foo.

I won't be able to work on this anymore, but it needs to be part of the release

I think merging #7784 would be more conservative.

@odersky
Copy link
Contributor Author

odersky commented Dec 17, 2019

@smarter I see what you mean. That aspect did not change. So, basically, outline parsing with nested packages was broken and is still broken now. #7784 "fixes" this by not using the feature in our own codebase. But others will run into the same problem.

So I think we should either fix it or disable it (by issuing a "not implemented error") before the release.

@smarter
Copy link
Member

smarter commented Dec 17, 2019

So, basically, outline parsing with nested packages was broken and is still broken now.

But before this PR the same code would have lead to a warning "class A is in the wrong directory." and wouldn't have added A to foo. Is there some other scenario where current master would add the symbol anyway ?

@odersky
Copy link
Contributor Author

odersky commented Dec 17, 2019

But before this PR the same code would have lead to a warning "class A is in the wrong directory." and wouldn't have added A to foo.

That's not quite as bad, but still counts as broken. The problem is, #7784 changes the public API of the core Type and Expr types in a non-desirable way to circumvent our lack of support for nested packages. So I much prefer we fix it instead.

@odersky
Copy link
Contributor Author

odersky commented Dec 18, 2019

I see it's not so easy to fix, since there is quite involved logic in Namer for creating packages. It's not clear we want to push all that into the outline parser. So, let's close this and revive #7784.

@odersky odersky closed this Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants