You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@groovy.apache.org by GitBox <gi...@apache.org> on 2020/07/31 05:06:00 UTC
[GitHub] [groovy] paulk-asert opened a new pull request #1332: GROOVY-9666: ConcurrentModificationException when processing ModuleNo…
paulk-asert opened a new pull request #1332:
URL: https://github.com/apache/groovy/pull/1332
…de imports
backwards compatibility trumps potential increased use of immutability
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] paulk-asert commented on pull request #1332: GROOVY-9666: ConcurrentModificationException when processing ModuleNo…
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on pull request #1332:
URL: https://github.com/apache/groovy/pull/1332#issuecomment-667160358
I will put it back the way it was and add the test cases from above which work. We have had many long discussions about changing the AST data structures to be more immutable (or even more consistently mutable!) but it isn't something which is easy to half do. We can certainly re-look for 4 but it will be some work.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] paulk-asert edited a comment on pull request #1332: GROOVY-9666: ConcurrentModificationException when processing ModuleNo…
Posted by GitBox <gi...@apache.org>.
paulk-asert edited a comment on pull request #1332:
URL: https://github.com/apache/groovy/pull/1332#issuecomment-667134508
The following all fail for me in 3.0.5:
```
import org.codehaus.groovy.ast.ModuleNode
import org.codehaus.groovy.control.SourceUnit
import org.codehaus.groovy.ast.ClassHelper
def mn = new ModuleNode((SourceUnit)null)
mn.addStarImport('foo.bar')
assert mn.starImports.size() == 1
for (starImport in mn.starImports) {
mn.addStarImport(starImport.packageName.toUpperCase())
}
assert mn.starImports.size() == 2
mn.addStaticStarImport('Integer', ClassHelper.Integer_TYPE)
mn.addStaticStarImport('Long', ClassHelper.Integer_TYPE)
assert mn.staticStarImports.size() == 2
for (staticStarImport in mn.staticStarImports) {
mn.addStaticStarImport('Short', ClassHelper.Long_TYPE)
}
assert mn.staticStarImports.size() == 4
mn.addStaticImport(ClassHelper.Integer_TYPE, 'MIN_VALUE', 'MIN_VALUE')
mn.addStaticImport(ClassHelper.Long_TYPE, 'MIN_VALUE', 'MIN_VALUE')
assert mn.staticImports.size() == 2
for (staticImport in mn.staticImports) {
mn.addStaticImport(staticImport.value.type, 'MAX_VALUE', 'MAX_VALUE')
}
assert mn.staticImports.size() == 4
mn.addImport('Integer', ClassHelper.Integer_TYPE)
assert mn.imports.size() == 1
for (importNode in mn.imports) {
mn.addImport('Natural', ClassHelper.Integer_TYPE)
}
assert mn.imports.size() == 2
mn.addMethod(null)
assert mn.methods.size() == 1
for (method in mn.methods) {
mn.addMethod(null)
}
assert mn.methods.size() == 2
```
You are correct that most also failed on 3.0.4 but if we are going to support that style for `addImport`, we should probably be consistent.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] paulk-asert edited a comment on pull request #1332: GROOVY-9666: ConcurrentModificationException when processing ModuleNo…
Posted by GitBox <gi...@apache.org>.
paulk-asert edited a comment on pull request #1332:
URL: https://github.com/apache/groovy/pull/1332#issuecomment-667134508
The following all fail for me in 3.0.5:
```
import org.codehaus.groovy.ast.ModuleNode
import org.codehaus.groovy.control.SourceUnit
import org.codehaus.groovy.ast.ClassHelper
def mn = new ModuleNode((SourceUnit)null)
mn.addStarImport('foo.bar')
assert mn.starImports.size() == 1
for (starImport in mn.starImports) {
mn.addStarImport(starImport.packageName.toUpperCase())
}
assert mn.starImports.size() == 2
mn.addStaticStarImport('Integer', ClassHelper.Integer_TYPE)
mn.addStaticStarImport('Long', ClassHelper.Integer_TYPE)
assert mn.staticStarImports.size() == 2
for (staticStarImport in mn.staticStarImports) {
mn.addStaticStarImport('Short', ClassHelper.Long_TYPE)
}
assert mn.staticStarImports.size() == 4
mn.addStaticImport(ClassHelper.Integer_TYPE, 'MIN_VALUE', 'MIN_VALUE')
mn.addStaticImport(ClassHelper.Long_TYPE, 'MIN_VALUE', 'MIN_VALUE')
assert mn.staticImports.size() == 2
for (staticImport in mn.staticImports) {
mn.addStaticImport(staticImport.value.type, 'MAX_VALUE', 'MAX_VALUE')
}
assert mn.staticImports.size() == 4
mn.addImport('Integer', ClassHelper.Integer_TYPE)
assert mn.imports.size() == 1
for (importNode in mn.imports) {
mn.addImport('Natural', ClassHelper.Integer_TYPE)
}
assert mn.imports.size() == 2
mn.addMethod(null)
assert mn.methods.size() == 1
for (method in mn.methods) {
mn.addMethod(null)
}
assert mn.methods.size() == 2
```
You are correct that most also failed on 3.0.4 but if we are going to support that style for `addImport`, we should probably be consistent.
It is messy though. Just looking at 2_5_X, we actually return the original collections for `methods`, `starImports`, `staticStarImports` and `staticImports`. So, even returning a copy is a breaking change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] paulk-asert edited a comment on pull request #1332: GROOVY-9666: ConcurrentModificationException when processing ModuleNo…
Posted by GitBox <gi...@apache.org>.
paulk-asert edited a comment on pull request #1332:
URL: https://github.com/apache/groovy/pull/1332#issuecomment-667134508
The following all fail for me in 3.0.5:
```
import org.codehaus.groovy.ast.ModuleNode
import org.codehaus.groovy.control.SourceUnit
import org.codehaus.groovy.ast.ClassHelper
def mn = new ModuleNode((SourceUnit)null)
mn.addStarImport('foo.bar')
assert mn.starImports.size() == 1
for (starImport in mn.starImports) {
mn.addStarImport(starImport.packageName.toUpperCase())
}
assert mn.starImports.size() == 2
mn.addStaticStarImport('Integer', ClassHelper.Integer_TYPE)
mn.addStaticStarImport('Long', ClassHelper.Integer_TYPE)
assert mn.staticStarImports.size() == 2
for (staticStarImport in mn.staticStarImports) {
mn.addStaticStarImport('Short', ClassHelper.Long_TYPE)
}
assert mn.staticStarImports.size() == 4
mn.addStaticImport(ClassHelper.Integer_TYPE, 'MIN_VALUE', 'MIN_VALUE')
mn.addStaticImport(ClassHelper.Long_TYPE, 'MIN_VALUE', 'MIN_VALUE')
assert mn.staticImports.size() == 2
for (staticImport in mn.staticImports) {
mn.addStaticImport(staticImport.value.type, 'MAX_VALUE', 'MAX_VALUE')
}
assert mn.staticImports.size() == 4
mn.addImport('Integer', ClassHelper.Integer_TYPE)
assert mn.imports.size() == 1
for (importNode in mn.imports) {
mn.addImport('Natural', ClassHelper.Integer_TYPE)
}
assert mn.imports.size() == 2
mn.addMethod(null)
assert mn.methods.size() == 1
for (method in mn.methods) {
mn.addMethod(null)
}
assert mn.methods.size() == 2
```
You are correct that most also failed on 3.0.4 but if we are going to support that style for `addImport`, we should probably be consistent.
It is messy though. Just looking at 3_0_X up until 3.0.4 and 2_5_X, we actually return the original collections for `methods`, `starImports`, `staticStarImports` and `staticImports`. So, even returning a copy is a breaking change, i.e.
```
mn.addStarImport('foo.bar')
assert mn.starImports.size() == 1
assert mn.starImports*.text == ['import foo.bar*']
// simulate xform that manipulates imports
def copy = mn.starImports.clone()
mn.starImports.clear()
for (starImport in copy) {
mn.addStarImport(starImport.packageName.toUpperCase())
}
assert mn.starImports.size() == 1
assert mn.starImports*.text == ['import FOO.BAR*']
```
So, I think we need to just put it back the way it was.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] danielsun1106 commented on pull request #1332: GROOVY-9666: ConcurrentModificationException when processing ModuleNo…
Posted by GitBox <gi...@apache.org>.
danielsun1106 commented on pull request #1332:
URL: https://github.com/apache/groovy/pull/1332#issuecomment-667508577
Merged. Thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] eric-milles commented on pull request #1332: GROOVY-9666: ConcurrentModificationException when processing ModuleNo…
Posted by GitBox <gi...@apache.org>.
eric-milles commented on pull request #1332:
URL: https://github.com/apache/groovy/pull/1332#issuecomment-667094288
Could `ModuleNode#addImport` do copy-on-write instead?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] eric-milles edited a comment on pull request #1332: GROOVY-9666: ConcurrentModificationException when processing ModuleNo…
Posted by GitBox <gi...@apache.org>.
eric-milles edited a comment on pull request #1332:
URL: https://github.com/apache/groovy/pull/1332#issuecomment-667094288
Could `ModuleNode#addImport` do copy-on-write instead? That seems to be the only source of backwards compatibility issues:
```java
private void visit(ModuleNode moduleNode, SourceUnit sourceUnit) {
for (ImportNode importNode : moduleNode.getImports()) {
if (shouldTransformClass(importNode.getType())) {
moduleNode.addImport(importNode.getAlias(), rewriteType(importNode.getType()));
}
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] paulk-asert commented on pull request #1332: GROOVY-9666: ConcurrentModificationException when processing ModuleNo…
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on pull request #1332:
URL: https://github.com/apache/groovy/pull/1332#issuecomment-667134508
The following all fail for me in 3.0.5:
```
import org.codehaus.groovy.ast.ModuleNode
import org.codehaus.groovy.control.SourceUnit
import org.codehaus.groovy.ast.ClassHelper
def mn = new ModuleNode((SourceUnit)null)
mn.addStarImport('foo.bar')
assert mn.starImports.size() == 1
for (starImport in mn.starImports) {
mn.addStarImport(starImport.packageName.toUpperCase())
}
assert mn.starImports.size() == 2
mn.addStaticStarImport('Integer', ClassHelper.Integer_TYPE)
mn.addStaticStarImport('Long', ClassHelper.Integer_TYPE)
assert mn.staticStarImports.size() == 2
for (staticStarImport in mn.staticStarImports) {
mn.addStaticStarImport('Short', ClassHelper.Long_TYPE)
}
assert mn.staticStarImports.size() == 4
mn.addStaticImport(ClassHelper.Integer_TYPE, 'MIN_VALUE', 'MIN_VALUE')
mn.addStaticImport(ClassHelper.Long_TYPE, 'MIN_VALUE', 'MIN_VALUE')
assert mn.staticImports.size() == 2
for (staticImport in mn.staticImports) {
mn.addStaticImport(staticImport.value.type, 'MAX_VALUE', 'MAX_VALUE')
}
assert mn.staticImports.size() == 4
mn.addImport('Integer', ClassHelper.Integer_TYPE)
assert mn.imports.size() == 1
for (importNode in mn.imports) {
mn.addImport('Natural', ClassHelper.Integer_TYPE)
}
assert mn.imports.size() == 2
mn.addMethod(null)
assert mn.methods.size() == 1
for (method in mn.methods) {
mn.addMethod(null)
}
assert mn.methods.size() == 2
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] paulk-asert edited a comment on pull request #1332: GROOVY-9666: ConcurrentModificationException when processing ModuleNo…
Posted by GitBox <gi...@apache.org>.
paulk-asert edited a comment on pull request #1332:
URL: https://github.com/apache/groovy/pull/1332#issuecomment-667134508
The following all fail for me in 3.0.5:
```
import org.codehaus.groovy.ast.ModuleNode
import org.codehaus.groovy.control.SourceUnit
import org.codehaus.groovy.ast.ClassHelper
def mn = new ModuleNode((SourceUnit)null)
mn.addStarImport('foo.bar')
assert mn.starImports.size() == 1
for (starImport in mn.starImports) {
mn.addStarImport(starImport.packageName.toUpperCase())
}
assert mn.starImports.size() == 2
mn.addStaticStarImport('Integer', ClassHelper.Integer_TYPE)
mn.addStaticStarImport('Long', ClassHelper.Integer_TYPE)
assert mn.staticStarImports.size() == 2
for (staticStarImport in mn.staticStarImports) {
mn.addStaticStarImport('Short', ClassHelper.Long_TYPE)
}
assert mn.staticStarImports.size() == 4
mn.addStaticImport(ClassHelper.Integer_TYPE, 'MIN_VALUE', 'MIN_VALUE')
mn.addStaticImport(ClassHelper.Long_TYPE, 'MIN_VALUE', 'MIN_VALUE')
assert mn.staticImports.size() == 2
for (staticImport in mn.staticImports) {
mn.addStaticImport(staticImport.value.type, 'MAX_VALUE', 'MAX_VALUE')
}
assert mn.staticImports.size() == 4
mn.addImport('Integer', ClassHelper.Integer_TYPE)
assert mn.imports.size() == 1
for (importNode in mn.imports) {
mn.addImport('Natural', ClassHelper.Integer_TYPE)
}
assert mn.imports.size() == 2
mn.addMethod(null)
assert mn.methods.size() == 1
for (method in mn.methods) {
mn.addMethod(null)
}
assert mn.methods.size() == 2
```
You are correct that most also failed on 3.0.4 but if we are going to support that style for `addImport`, we should probably be consistent.
It is messy though. Just looking at 2_5_X, we actually return the original collections for `methods`, `starImports`, `staticStarImports` and `staticImports`. So, even returning a copy is a braking change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] danielsun1106 merged pull request #1332: GROOVY-9666: ConcurrentModificationException when processing ModuleNo…
Posted by GitBox <gi...@apache.org>.
danielsun1106 merged pull request #1332:
URL: https://github.com/apache/groovy/pull/1332
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] eric-milles commented on pull request #1332: GROOVY-9666: ConcurrentModificationException when processing ModuleNo…
Posted by GitBox <gi...@apache.org>.
eric-milles commented on pull request #1332:
URL: https://github.com/apache/groovy/pull/1332#issuecomment-667101625
It is only the plain imports that have this issue because they were previously managed by a map and returned as a list. The other types of imports were returning the internal representation (list for list, map for map).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] paulk-asert commented on pull request #1332: GROOVY-9666: ConcurrentModificationException when processing ModuleNo…
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on pull request #1332:
URL: https://github.com/apache/groovy/pull/1332#issuecomment-669138850
> > even returning a copy is a breaking change
>
> That's my take. Since there is little chance of avoiding a breaking change, why not stick with returning immutables and let Vert.x adjust to making a copy to iterate over or performing changes after iteration is completed. I don't think the design was to return something for modification; that's exposing the implementation of `ModuleNode`. The emergent behavior that `getImports()` could be iterated over and `addImport(...)` could be called was not intended IMO.
>
> Worst case, could 3.x get the change just for `getImports()` and master be left as is?
@eric-milles As you will have seen, I reverted everything for now. It would be good to make these all consistent in Groovy 4 but we should provide a consistent approach across all AST nodes and a considered approach for mutating and have any breaking changes (if needed) documented in the release notes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] eric-milles commented on pull request #1332: GROOVY-9666: ConcurrentModificationException when processing ModuleNo…
Posted by GitBox <gi...@apache.org>.
eric-milles commented on pull request #1332:
URL: https://github.com/apache/groovy/pull/1332#issuecomment-667153834
> even returning a copy is a breaking change
That's my take. Since there is little chance of avoiding a breaking change, why not stick with returning immutables and let Vert.x adjust to making a copy to iterate over or performing changes after iteration is completed. I don't think the design was to return something for modification; that's exposing the implementation of `ModuleNode`. The emergent behavior that `getImports()` could be iterated over and `addImport(...)` could be called was not intended IMO.
Worst case, could 3.x get the change just for `getImports()` and master be left as is?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org