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

Fix #7650: Drop stale toplevel definitions #7652

Merged
merged 16 commits into from
Dec 11, 2019

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Dec 1, 2019

If there are several toplevel definitions of the same name in different wrapper objects, drop all but the definition that was compiled last. This mimics the behavior for classes and objects.

As a consequence, it becomes impossible to have overloaded toplevel definitions in different files, as the alternatives in the last-compiled file always replace the alternatives compiled earlier.

val pmembers = pcls.computeNPMembersNamed(name).filterWithPredicate { d =>
// Drop members of `Any` and `Object`, as well as top-level definitions
// in the empty package that are not defined in the current run.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the code below is related to dropping definitions in the empty package ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, that was left from the previous abandoned PR.

@@ -2122,8 +2122,6 @@ object SymDenotations {
case pcls :: pobjs1 =>
if (pcls.isCompleting) recur(pobjs1, acc)
else {
// A package object inherits members from `Any` and `Object` which
// should not be accessible from the package prefix.
Copy link
Member

Choose a reason for hiding this comment

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

The original comment here seems useful to me and could be restored.

)
if compiledNow.exists then compiledNow
else
val youngest = d.aggregate(_.symbol.associatedFile.lastModified, _ max _)
Copy link
Member

Choose a reason for hiding this comment

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

I'm strongly against any kind of merging logic that takes file modification date into account: there's many situations where this would go wrong, and it adds a new source of non-determinism to compilation. Instead, we can emit an error if overloaded definitions come from multiple files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That alone won't help. You still need the modification dates to get rid of junk in old classfiles. Otherwise any conflict would be an error which would be super annoying since often people do not even know how to clean out the output directory! So they are stuck with an error they can't fix.

Show that toplevel definitions on several files do not
combine as overloaded denotations. Instead, the youngest
definition wins.
Toplevel overloading tests are no longer supported. The toplevel doubledef
test works only if two generated class files have the same modification date,
which changes from run to run.
Copy link
Contributor

@anatoliykmetyuk anatoliykmetyuk left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM at the current stage

@@ -42,3 +42,6 @@ in a source file `src.scala`, it could be invoked from the command line using a
"program name" is mangled it is recommended to always put `main` methods in explicitly named objects.

**Note 3:** The notion of `private` is independent of whether a definition is wrapped or not. A `private` toplevel definition is always visible from everywhere in the enclosing package.

**Note 4:** If several wrapper objects contain toplevel definitions with the same name,
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this is true only for separate compilation. If you have several files, each with an overloaded version of a method, and compile them in one run, the overload will work. If you compile them separately, only the last compiled method will be visible.

Invalidate enclosing package's member cache when entering a toplevel definition in
a wrapper package object.
@odersky odersky force-pushed the fix-#7650b branch 3 times, most recently from 5c9ff47 to f7c25f7 Compare December 7, 2019 18:46
@odersky odersky force-pushed the fix-#7650b branch 2 times, most recently from 873e423 to 27ab7ae Compare December 7, 2019 21:49
Report an error if toplevel denotations the same name with come
from multiple files that have the same modification date.

The error was reported in some runs for toplevel/stale when run locally
since the files for A_1.scala and B_2.scala were generated too fast so they
ended up with the same modification date. B_2.scala has now been beefed
up to give the compiler more work to do, so that the race is avoided.
@odersky
Copy link
Contributor Author

odersky commented Dec 8, 2019

@smarter Everything we discussed is now implemented. You get warnings if competing toplevel definitions come from different directories. I was not able to do a test for this (won't have the time to do it). If we feel a test is needed, let's open an issue.

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