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

Dynamic is broken. #4536

Closed
scabug opened this Issue May 2, 2011 · 11 comments

Comments

Projects
None yet
2 participants
@scabug
Copy link

scabug commented May 2, 2011

applyDynamic goes into an infinite loop, calling "applyDynamic" on itself. For instance, try the pastie at http://pastie.org/pastes/1469174/text . But my guess is anything will be the same.

@scabug

This comment has been minimized.

Copy link

scabug commented May 2, 2011

Imported From: https://issues.scala-lang.org/browse/SI-4536?orig=1
Reporter: @paulp
Attachments:

@scabug

This comment has been minimized.

Copy link

scabug commented May 3, 2011

@dragos said:
I guess Martin earned a new ticket.

@scabug

This comment has been minimized.

Copy link

scabug commented Aug 3, 2011

@dcsobral said:
Let's be clear about what is happening here: there's no "applyDynamic" defined on the class in the example. So, when the compiler sees that a method does not exist, it tries to find the applyDynamic method to handle it. Since it doesn't exist, it tries to find the applyDynamic method to handle the applyDynamic method, and on and on.

The compiler should not do that. Furthermore, IMHO, any non-abstract class extending Dynamic should implement applyDynamic or an error should be returned.

@scabug

This comment has been minimized.

Copy link

scabug commented Sep 29, 2011

@jsalvata said:
The attached patch implements a solution by:

(1) Preventing the compiler running away by stopping it dynamically applying a call to "applyDynamic". This is sufficient to stop this bug proper, but puts the weight on the client, as the error only shows up at the point of call.

(2) Forbidding any subclassing of scala.Dynamic without declaring an applyDynamic member -- even traits or abstract classes. Forbidding it only for concrete classes is not sufficient because intermediate abstract classes would cause the error in (1) to come up, which could be pretty mysterious if it's a library class and the user doesn't even know about Dynamic.

Note that with change (2), the only way to run into (1) will be to call a dynamic method on scala.Dynamic directly or some class file compiled with an older version of the compiler.

Please review the patch carefully both in concept and in implementation, as I am not a computer language theorist and it's my first attempt at changing the scala compiler. Tomorrow I'll try to find out how to add test cases and create some for this change.

@scabug

This comment has been minimized.

Copy link

scabug commented Sep 29, 2011

@jsalvata said (edited on Sep 29, 2011 6:51:21 AM UTC):
I've realized that condition (2) can be applied without loss of generality as there is no reason to mix the Dynamic trait into the hierarchy until applyDynamic is first declared, possibly abstract.

@scabug

This comment has been minimized.

Copy link

scabug commented Sep 29, 2011

@jsalvata said:
Updated patch with better error reporting + some tests.

@scabug

This comment has been minimized.

Copy link

scabug commented Oct 1, 2011

@jsalvata said (edited on Oct 2, 2011 5:19:50 PM UTC):
Following recommendation from Paul Phillips at scala-internals (thanks, Paul!), I've proposed the patch through github: scala/scala#98

@scabug

This comment has been minimized.

Copy link

scabug commented May 3, 2012

@paulp said:
I don't know what happened, but that link to github is to an xml equality patch. Does the patch still exist somewhere?

@scabug

This comment has been minimized.

Copy link

scabug commented May 3, 2012

@paulp said:
It looks like Alex closed this yesterday, but it must have been a mistake.

@scabug

This comment has been minimized.

Copy link

scabug commented May 3, 2012

@jsalvata said:
FWIW, the above-mentioned patch is now in this URL: Apparently repositories were renamed: scala/legacy-svn-scala#98

"U" stands for "Universal", but apparently the time dimension is not included.

@scabug

This comment has been minimized.

Copy link

scabug commented May 4, 2012

@paulp said:
As of M3, this is no longer an issue; however you can't call dynamic methods on anything unless it can find the implementation, i.e. you can't call anything on Dynamic. I don't know if this is intentional - I started a thread on scala-language. For the moment I will close this as fixed, but will reopen if events say otherwise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment