SI-5543 Constructor default argument expr wrongly scoped #317
Conversation
|
Many thanks for the patch! I reviewed the changes and the fix looks correct to me. I'll add some comments to the code, let me know if you have time to make these changes. Otherwise I can also make the changes and do a new pull request myself. For the final pull request, I think it could be a single commit containing all the changes. |
|
OK, I'll fix the cautious comment to say SI-5543. I think the normal scalac style for that comment is: // TODO: ok? I don't like using a name compare instead of a flag, but that could be a future enhancement. (INCONSTRUCTOR might do, but it's already overloaded as LABEL.) The test case already checks that A.init is allowed. Do you mean test case class where there is more method synthesis? You prefer one commit per pull. Can I do git rebase -i and squash my commits? Maybe the pull request doesn't care? It looks like github workflow will auto-close the pull req when the commits are merged. Well, I'll see what happens. Thanks again for reviewing. |
|
So the answer is that the pull request doesn't care if you rebase, but we care. I apologize for losing your code comments. They are now history; or rather, they are no longer history. Live and learn. Let the record show that you approved renaming init$default to init$$default but suggested a test case; I asked above if you mean, a test case for "case". And you asked me to amend a spurious comment. Let me know if you'd like me to do more, short of erasing the past. Time travel truly is perilous. |
|
Many thanks for the changes. I overlooked that you already tested the "init" method in your test case, so that's perfectly fine. I agree a flag would be nice, but for now I think it's good as it is. Having many flags for rare cases is also not ideal. No problem about losing my comments :-) |
|
Can you verify you have signed and submitted the contributor agreement at http://www.scala-lang.org/sites/default/files/contributor_agreement.pdf ? |
|
I'll do that asap. This bug fix has no original content. I was going to suggest that under copyright law it falls under the rubric parody, but a quick check shows they have a better phrase to describe a bug fix: "incidental and fortuitous reproduction." ("Fortuitous" has the secondary connotation of an improved outcome.) |
|
Danielle Chamberlain has confirmed receipt. Since the pull is still open, I guess this one-line fix didn't cause the memory leak. |
|
You DID cause the memory leak, I know it. The only thing I don't like about this is the perpetuation of ad-hoc workarounds to name mangling collisions. If you were looking for something super useful to do, analyzing all the transformations we inflict on names - especially where correctness depends on it - could be that super useful thing. |
|
Yes. I commented on SI-5543 that the ad hocity needs guidance. Let alone, does anyone care if I break binary compatibility here? Also fragile (and maybe inefficient) is doing a strcmp for detecting what I already know is a special, flaggable method. I'll take you up on your challenge to my utility. That will greatly help my javap/scalap skills. Another utility: something to diff the before and after trees when I tweak a line of code. Like -Ybrowse, but just show a nice picture highlighting what changed from previous run. Someone linked to a tool that shows javascript in right-hand window and graphics/animation at left, where changes are <--> linked. A goal would be for the IDE to show me which code (or at least phase) is responsible for, say, injecting $workaround$init$cycle$. Another utility I needed was for the ML discussion of SI-3452 about generic sigs. I'm looking at awesome, but I'm inclined to call it plasma for paulp's little asm accommodation (where little is ironic understatement). It also fits because plasma (like plastic) has to do with something that can be molded or shaped. |
|
jenkins job pr-scala-testsuite-linux-opt: true - https://jenkins.typesafe.com/job/pr-scala-testsuite-linux-opt/1/ |
Reverts name unenmanglement that was objectionable in the previous patch commit 47bfd74.
This patch fixes the motivating bug by detecting when a method is the default arg getter for a constructor parameter. That requires fixing a secondary bug where an arbitrary string was used to encode <init> in lieu of <init>.encode. There is no speculative mangling.
|
I'm not sure this history is still useful. However, the fix is still useful. I changed the ad-hoc name mangling to standard encoding. (The code moved from NameManglers to StdNames, after all.) I pushed the two line fix to a new branch and I can make a clean pull request from there if that's the right thing to do. |
This is a fix of limited scope (pun unintended, but I'll take it).
https://issues.scala-lang.org/browse/SI-5543
One line for the context bug, and a couple of lines to encapsulate detecting a default-getter for a ctor.
This change includes renaming init$default$ to init$$default$ to avoid collisions with default-getters for an ordinary method on the module named init.