Fix #9772: Normalize value definitions #9836

tudorv91 · 2020-09-20T16:02:56Z

Disallow extension_ name prefix for value definitions
Make extension variable names illegal (addressing Cannot implement setter method for var extension #9926)

dottybot

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Have an awesome day! ☀️

bishabosha

Thank you for opening this pull request, however I think this needs more consideration, for example, it is illegal to define object extension_foo, which suggests that val definitions should also be covered by this check, or else it is a bug for objects to cause this error.

What do you think @smarter?

compiler/src/dotty/tools/dotc/ast/Desugar.scala

bishabosha · 2020-10-01T13:38:36Z

This pull request should probably also check the name of the generated setter, e.g. as shown in #9926 extension_= will be blocked by the compiler, but this PR would allow var extension

bishabosha · 2020-10-01T13:39:08Z

conflicts also need to be resolved. @tudorv91 are you able to look at this?

tudorv91 · 2020-10-01T17:17:42Z

@bishabosha sure I'll have a look

compiler/src/dotty/tools/dotc/ast/Desugar.scala

tudorv91 · 2020-10-03T07:36:55Z

This pull request should probably also check the name of the generated setter, e.g. as shown in #9926 extension_= will be blocked by the compiler, but this PR would allow var extension

@bishabosha Implemented the extension name restriction for variables definitions. I have considered a more general restriction (that would also apply to immutable values), but decided against in fear of this limitation's effects on expressivity and legacy code. What do you think?

bishabosha

looks pretty good, thank you, I have a few suggestions

compiler/src/dotty/tools/dotc/core/NameOps.scala

compiler/src/dotty/tools/dotc/ast/Desugar.scala

tests/neg/illegal-extension.scala

bishabosha · 2020-10-14T08:39:31Z

test performance please

dottybot · 2020-10-14T08:40:48Z

performance test scheduled: 1 job(s) in queue, 0 running.

bishabosha · 2020-10-14T08:47:50Z

compiler/src/dotty/tools/dotc/ast/Desugar.scala

-      && (!mods.is(Private) || ctx.owner.is(Trait) || ctx.owner.isPackageObject)
-    if (setterNeeded) {
+
+    val valName = normalizeName(vdef, tpt).asTermName


perhaps isSetterNeeded could be a default parameter for normalizeName to avoid calling it twice, and then swap the conditions in the Valdef branch (case vdef: ValDef if isSetterNeeded && name.isExtension), but if the performance isn't impacted that much this is minor

Yes, I considered that as well, but decided against it in order not to particularize the arg list of normalizeName with a param that is relevant to one branch. The second call would happen only if the valDef's name is extension, so I would expect it to be highly exceptional

bishabosha

Thank you for taking this on, I think this looks good.

bishabosha · 2020-10-14T08:51:26Z

.jvmopts

@@ -1,4 +1,6 @@
 -Xss1m
 -Xms512m
-Xmx1200m
+-Xmx4096m


also are the changes to this file necessary?

Hmm, strange that it appears as a diff. This is what's on master, no?

Rebased against the upstream and it's fine now

dottybot · 2020-10-14T09:57:42Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/9836/ to see the changes.

Benchmarks is based on merging with master (68a7c03)

bishabosha · 2020-10-14T10:02:09Z

test performance please

dottybot · 2020-10-14T10:03:09Z

performance test scheduled: 1 job(s) in queue, 0 running.

dottybot · 2020-10-14T11:20:45Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/9836/ to see the changes.

Benchmarks is based on merging with master (68a7c03)

bishabosha · 2020-10-14T11:43:41Z

pretty strange error in the CI for test_bootstrapped:

bootstrapped/test;sjsSandbox/run;sjsSandbox/test;sjsJUnitTests/test;sjsCompilerTests/test ;configureIDE"
Exception in thread "sbt-parser-init-thread" java.lang.ExceptionInInitializerError
	at sbt.internal.parser.SbtParserInit$$anon$2.run(SbtParser.scala:171)
Caused by: scala.reflect.internal.FatalError: Error accessing /root/.sbt/0.13/java9-rt-ext-ubuntu_11_0_7/rt.jar
	at scala.tools.nsc.classpath.AggregateClassPath.$anonfun$list$3(AggregateClassPath.scala:99)
	at scala.collection.Iterator.foreach(Iterator.scala:941)
	at scala.collection.Iterator.foreach$(Iterator.scala:941)
	at scala.collection.AbstractIterator.foreach(Iterator.scala:1429)
	at scala.collection.IterableLike.foreach(IterableLike.scala:74)
	at scala.collection.IterableLike.foreach$(IterableLike.scala:73)
	at scala.collection.AbstractIterable.foreach(Iterable.scala:56)
	at scala.tools.nsc.classpath.AggregateClassPath.list(AggregateClassPath.scala:87)
	at scala.tools.nsc.util.ClassPath.list(ClassPath.scala:36)
	at scala.tools.nsc.util.ClassPath.list$(ClassPath.scala:36)
	at scala.tools.nsc.classpath.AggregateClassPath.list(AggregateClassPath.scala:30)
	at scala.tools.nsc.symtab.SymbolLoaders$PackageLoader.doComplete(SymbolLoaders.scala:284)
	at scala.tools.nsc.symtab.SymbolLoaders$SymbolLoader.complete(SymbolLoaders.scala:230)
	at scala.reflect.internal.Symbols$Symbol.info(Symbols.scala:1542)
	at scala.reflect.internal.Mirrors$RootsBase.init(Mirrors.scala:257)
	at scala.tools.nsc.Global.rootMirror$lzycompute(Global.scala:74)
	at scala.tools.nsc.Global.rootMirror(Global.scala:72)
	at scala.tools.nsc.Global.rootMirror(Global.scala:44)
	at scala.reflect.internal.Definitions$DefinitionsClass.ObjectClass$lzycompute(Definitions.scala:295)
	at scala.reflect.internal.Definitions$DefinitionsClass.ObjectClass(Definitions.scala:295)
	at scala.reflect.internal.Definitions$DefinitionsClass.init(Definitions.scala:1480)
	at scala.tools.nsc.Global$Run.<init>(Global.scala:1199)
	at sbt.internal.parser.SbtParser$.<init>(SbtParser.scala:121)
	at sbt.internal.parser.SbtParser$.<clinit>(SbtParser.scala)
	... 1 more
Caused by: java.io.IOException: Error accessing /root/.sbt/0.13/java9-rt-ext-ubuntu_11_0_7/rt.jar
	at scala.reflect.io.FileZipArchive.scala$reflect$io$FileZipArchive$$openZipFile(ZipArchive.scala:141)
	at scala.reflect.io.FileZipArchive.root$lzycompute(ZipArchive.scala:179)
	at scala.reflect.io.FileZipArchive.root(ZipArchive.scala:176)
	at scala.reflect.io.FileZipArchive.allDirs$lzycompute(ZipArchive.scala:212)
	at scala.reflect.io.FileZipArchive.allDirs(ZipArchive.scala:212)
	at scala.tools.nsc.classpath.ZipArchiveFileLookup.findDirEntry(ZipArchiveFileLookup.scala:76)
	at scala.tools.nsc.classpath.ZipArchiveFileLookup.list(ZipArchiveFileLookup.scala:63)
	at scala.tools.nsc.classpath.ZipArchiveFileLookup.list$(ZipArchiveFileLookup.scala:62)
	at scala.tools.nsc.classpath.ZipAndJarClassPathFactory$ZipArchiveClassPath.list(ZipAndJarFileLookupFactory.scala:56)
	at scala.tools.nsc.classpath.AggregateClassPath.$anonfun$list$3(AggregateClassPath.scala:91)
	... 24 more
Caused by: java.util.zip.ZipException: zip END header not found
	at java.base/java.util.zip.ZipFile$Source.zerror(ZipFile.java:1535)
	at java.base/java.util.zip.ZipFile$Source.findEND(ZipFile.java:1436)
	at java.base/java.util.zip.ZipFile$Source.initCEN(ZipFile.java:1443)
	at java.base/java.util.zip.ZipFile$Source.<init>(ZipFile.java:1274)
	at java.base/java.util.zip.ZipFile$Source.get(ZipFile.java:1237)
	at java.base/java.util.zip.ZipFile$CleanableResource.<init>(ZipFile.java:727)
	at java.base/java.util.zip.ZipFile$CleanableResource.get(ZipFile.java:844)
	at java.base/java.util.zip.ZipFile.<init>(ZipFile.java:247)
	at java.base/java.util.zip.ZipFile.<init>(ZipFile.java:177)
	at java.base/java.util.zip.ZipFile.<init>(ZipFile.java:191)
	at scala.reflect.io.FileZipArchive.scala$reflect$io$FileZipArchive$$openZipFile(ZipArchive.scala:138)
	... 33 more
[info] welcome to sbt 1.3.12 (Ubuntu Java 11.0.7)
Error:  java.lang.NoClassDefFoundError: Could not initialize class sbt.internal.parser.SbtParser$
Error:  Use 'last' for the full log.
Project loading failed: (r)etry, (q)uit, (l)ast, or (i)gnore? 
Error: Process completed with exit code 1.

smarter · 2020-10-14T11:56:51Z

pretty strange error in the CI for test_bootstrapped:

This is likely fixed by sbt/zinc#915

bishabosha · 2020-10-14T11:58:40Z

would make sense seeing as this suddenly happened after .jvmops stack size and heap were reset to master branch

* Restrict names for mutable values which lead to setter generation during desugaring

* Make `extension` variable names illegal * Fix scala#9772: Disallow `extension_` for immutable values

Co-authored-by: Jamie Thompson <bishbashboshjt@gmail.com>

tudorv91 · 2020-10-14T21:24:56Z

@bishabosha I've rebased again and now it seems to be fine, especially if it was unrelated to my changes. Looking at the benchmarks, they don't seem to deviate significantly. What do you think?

bishabosha · 2020-10-15T08:31:30Z

thanks a lot :)

dottybot reviewed Sep 20, 2020

View reviewed changes

tudorv91 mentioned this pull request Sep 20, 2020

ValDef name is not normalised #9772

Closed

bishabosha self-assigned this Sep 21, 2020

bishabosha reviewed Sep 23, 2020

View reviewed changes

compiler/src/dotty/tools/dotc/ast/Desugar.scala Outdated Show resolved Hide resolved

bishabosha mentioned this pull request Oct 1, 2020

Cannot implement setter method for var extension #9926

Closed

tudorv91 force-pushed the fix-#9772 branch 5 times, most recently from 139ab3e to 305e5b7 Compare October 3, 2020 06:21

tudorv91 commented Oct 3, 2020

View reviewed changes

compiler/src/dotty/tools/dotc/ast/Desugar.scala Show resolved Hide resolved

bishabosha requested changes Oct 6, 2020

View reviewed changes

compiler/src/dotty/tools/dotc/core/NameOps.scala Outdated Show resolved Hide resolved

compiler/src/dotty/tools/dotc/ast/Desugar.scala Outdated Show resolved Hide resolved

tudorv91 requested a review from bishabosha October 6, 2020 17:51

tudorv91 force-pushed the fix-#9772 branch from 61b32da to c0491e0 Compare October 6, 2020 18:18

bishabosha reviewed Oct 8, 2020

View reviewed changes

tests/neg/illegal-extension.scala Outdated Show resolved Hide resolved

tudorv91 force-pushed the fix-#9772 branch 2 times, most recently from 21c8e26 to 76e4ef2 Compare October 11, 2020 18:12

tudorv91 requested a review from bishabosha October 11, 2020 18:21

bishabosha reviewed Oct 14, 2020

View reviewed changes

bishabosha approved these changes Oct 14, 2020

View reviewed changes

bishabosha reviewed Oct 14, 2020

View reviewed changes

tudorv91 force-pushed the fix-#9772 branch from 76e4ef2 to 2067973 Compare October 14, 2020 10:08

tudorv91 and others added 4 commits October 14, 2020 22:28

Fix scala#9772: Normalize value definitions

5899de2

* Restrict names for mutable values which lead to setter generation during desugaring

Fix scala#9926: Normalize value definitions

5402b85

* Make `extension` variable names illegal * Fix scala#9772: Disallow `extension_` for immutable values

Update compiler/src/dotty/tools/dotc/ast/Desugar.scala

a17b774

Co-authored-by: Jamie Thompson <bishbashboshjt@gmail.com>

Update compiler/src/dotty/tools/dotc/core/NameOps.scala

341afb3

Co-authored-by: Jamie Thompson <bishbashboshjt@gmail.com>

tudorv91 force-pushed the fix-#9772 branch from 2067973 to 341afb3 Compare October 14, 2020 19:28

bishabosha merged commit 63771ac into scala:master Oct 15, 2020

Kordyjan added this to the 3.0.0 milestone Aug 2, 2023

Fix #9772: Normalize value definitions #9836

Fix #9772: Normalize value definitions #9836

Uh oh!

Conversation

tudorv91 commented Sep 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dottybot left a comment

Choose a reason for hiding this comment

Uh oh!

bishabosha left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bishabosha commented Oct 1, 2020

Uh oh!

bishabosha commented Oct 1, 2020

Uh oh!

tudorv91 commented Oct 1, 2020

Uh oh!

Uh oh!

tudorv91 commented Oct 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bishabosha left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bishabosha commented Oct 14, 2020

Uh oh!

dottybot commented Oct 14, 2020

Uh oh!

bishabosha Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tudorv91 Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bishabosha left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bishabosha Oct 14, 2020

Choose a reason for hiding this comment

Uh oh!

tudorv91 Oct 14, 2020

Choose a reason for hiding this comment

Uh oh!

tudorv91 Oct 14, 2020

Choose a reason for hiding this comment

Uh oh!

dottybot commented Oct 14, 2020

Uh oh!

bishabosha commented Oct 14, 2020

Uh oh!

dottybot commented Oct 14, 2020

Uh oh!

dottybot commented Oct 14, 2020

Uh oh!

bishabosha commented Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smarter commented Oct 14, 2020

Uh oh!

bishabosha commented Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tudorv91 commented Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bishabosha commented Oct 15, 2020

Uh oh!

Uh oh!

tudorv91 commented Sep 20, 2020 •

edited

Loading

tudorv91 commented Oct 3, 2020 •

edited

Loading

bishabosha Oct 14, 2020 •

edited

Loading

tudorv91 Oct 14, 2020 •

edited

Loading

bishabosha left a comment •

edited

Loading

bishabosha commented Oct 14, 2020 •

edited

Loading

bishabosha commented Oct 14, 2020 •

edited

Loading

tudorv91 commented Oct 14, 2020 •

edited

Loading