Skip to content

Fix Tasty errors #1211

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

Merged
merged 21 commits into from
Apr 16, 2016
Merged

Fix Tasty errors #1211

merged 21 commits into from
Apr 16, 2016

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Apr 7, 2016

Fixes one of the errors reported in #1202. If I can fix more errors, I'll try to push those fixes on this PR later. Review by @VladimirNik .

odersky added 15 commits April 7, 2016 17:51
For explicit arguments of this(...) constrictor calls we have a special
context that hides members of the current class. But for implicit arguments
we did not. This led to implicit shadowing errors for
scala.collection.immutable.PagedSeq when secondary constructor
type parameters were fixed (as done in subsequent commits).
Previously, those were inferred from arguments, but this is wrong
because we implicitly assume that the type parameters of the constructor
and the type parameters of the class are the same.

I could not find a test that fails for this. But if you look at the
-Xprint:front output of pos/i941.scala, you notice that the
inferred argument to the this(...) call was `Nothing` where
it should have been `A`.
... of class parameters using the gadt mechanism. Previously they were encoded as aliases
by hardcoding alias bounds in the type parameter declaration, but that then leads to
weird behavior and failures in unpickling.

To make this work, we also need to propagate gadt bounds into the this-call context.

Test case in pickling/i1202a.scala.
`isTyper` is used to enable some error checking and handling, which
need not be done when in FromTasty.
The previous path name always had a "Simple(...)" wrapped around it.
If a file was loaded from TASTY, it can not still have a non-null source file,
since the source file is unpickled into the annotation of a top-level class.

Also, fix typo in previous commit.
ParamAccessor is not a pickled flag. This is not a problem for
normal parameter accessors which are pickled as PARAM fields. But setters
of parameter accessors also need to have the flag set (and Deferred reset).
SeqLiteral have an elemTpt, which was missing in format.
Two problems were fixed:

 - isJava needs to look at function symbol, not its type (references
   to Java methods get normal MethodTypes not JavMethodTypes)

 - we also need to handle the case where the repeated argument is
   wrspped in a type ascription.
@odersky
Copy link
Contributor Author

odersky commented Apr 8, 2016

Now the other errors reported in #1202 should also be fixed.

@VladimirNik
Copy link
Contributor

@odersky Thank you! LGTM

@@ -64,6 +64,9 @@ object FromTasty extends Driver {
}

class ReadTastyTreesFromClasses extends FrontEnd {

override def isTyper = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, this is not enough.
A lot of code checks for Phases.isAfterTyper that is entirely independent from this method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. But it does not matter for anyway. The change did not affect the fixes. I'll add a comment to explain better what isTyper is.

odersky added 5 commits April 8, 2016 15:04
This is the same as what Java does for its ClassFile attribute.
Instead of separate source file sections, pickle SourceFile as an
annotation of all toplevel classes.

We represent it like this anyway when reading back Tasty-defined classes.
This leads to an infinite cycle when trying to unpickling, because
the modifiers and annotations of a symbol are read before the symbol
itself is created.

See scala#1212 for the general case.
1) Move passing test to pickling
2) Add test case for scala#1212 in pending
3) Disable annotations/internal in pickling tests. They lead to a stable symbol error
   which is explainable (modifiers are read before symbol is created).
@DarkDimius
Copy link
Contributor

I'm not sure if change proposed in 5aa59ac is good for tooling.

It makes it very complicated for an external tool to match TASTY files with source files. As the tool would be required to be able to read the TASTY section.

Though it also has bright sides, as it would simplify storing TASTY for classes originating from different source files.

@odersky
Copy link
Contributor Author

odersky commented Apr 9, 2016

If necessary we could provide a method that returns the top level classes of a tasty file. Then we can read the source files from them. I think it is an advantage that A tasty file is not tied to one source.

Sent from my iPhone

On 09.04.2016, at 13:03, Dmitry Petrashko notifications@github.com wrote:

I'm not sure if change proposed in 5aa59ac is good for tooling.

It makes it very complicated for an external tool to match TASTY files with source files. As the tool would be required to be able to read the TASTY section.

Though it also has bright sides, as it would simplify storing TASTY for classes originating from different source files.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

@VladimirNik
Copy link
Contributor

I added commit with updates for TASTY tests (for dotty/src)

@odersky odersky merged commit 3ef4115 into scala:master Apr 16, 2016
@allanrenucci allanrenucci deleted the fix-#1202 branch December 14, 2017 16:59
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