groovy

Compilation should always fail when two methods exist in the same class with the same name but differing access levels

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Fixed
  • Affects Version/s: 1.8.4
  • Fix Version/s: 1.8.5, 2.0-beta-2
  • Component/s: None
  • Labels:
  • Number of attachments :
    0

Description

Currently, the check only fails under certain circumstatnces, but according to Jochen, it should always fail.

The following passes, but apparently should not:

class Repository {
  def find(String id) {}
  private <T> T find(Class<T> type, String id, boolean suppressNotFoundExceptions) { }
}

The following fails, as expected:

class Repository {
  def find(String id) {}
  private <T> T find(Class<T> type, String id, boolean suppressNotFoundExceptions = true) { }
}

Activity

Hide
Permalink
Cedric Champeau added a comment -

Patch is reverted in 1.8.6 because it broke some code which previously compiled in Grails. The fix is only on 2.0.x. Here's the discussion:

[11:40:55 CEST] Peter Ledbrook: Another one for the Groovy folks: http://pastebin.com/0FUCJzPa
[11:41:32 CEST] Peter Ledbrook: Recognise the error? This is from an existing Grails project that compiled fine with Grails 2.0, but fails with 2.0.1 (with Groovy 1.8.5)
[11:43:39 CEST] Cédric Champeau: yes, it happens when you have two covariant methods with different modifiers (private/public), Jochen fixed an issue with that recently
[11:43:54 CEST] Cédric Champeau: so that's why the warning occurs
[11:44:18 CEST] Peter Ledbrook: It's not a warning. It's an error and the compilation fails. Or has it been reverted to a warning for 1.8.6?
[11:44:32 CEST] Cédric Champeau: I mean error, there's no warning in Groovy :)
[11:45:07 CEST] Peter Ledbrook: Can I say that I think this is a bad change for a patch release?
[11:46:26 CEST] Cédric Champeau: You can, but I'm sure Jochen has good arguments :) (That's also a reason for the numbering scheme change, with the last number which should only be used for bugfixes starting from 2.0)
[11:47:27 CEST] Peter Ledbrook: Numbering scheme change? You mean from 1.9 -> 2.0? Or something else?
[11:47:42 CEST] Peter Ledbrook: Anyway, I believe that there is no good reason to introduce a breaking change into a patch release.
[11:47:43 CEST] Cédric Champeau: yes, 1.9 becoming 2.0.0
[11:48:08 CEST] Peter Ledbrook: It happened with Grails and it's a bad idea. We definitely don't want Grails 2.0.1 going out with a breaking change.
[11:49:35 CEST] Cédric Champeau: found the jira issue related to this change: https://jira.codehaus.org/browse/GROOVY-5193
[11:49:44 CEST] Cédric Champeau: considered as a bugfix
[11:51:40 CEST] Peter Ledbrook: Thanks. But it's still a breaking change and anyone upgrading from Grails 2.0 to 2.0.1 will suddenly hit this compilation error with Searchable. And who knows what other plugins.
[11:51:55 CEST] Peter Ledbrook: I'm not defending the code that's not compiling.
[11:51:59 CEST] Cédric Champeau: from what I read, it's still possible to remove the fix from 1.8.x branch and keep it in 2.0
[11:52:24 CEST] Peter Ledbrook: That would get my vote :) So is there no way to make it a warning?
[11:52:32 CEST] Cédric Champeau: unfortunately, no
[11:54:00 CEST] Peter Ledbrook: Ouch. Which means no one knows that code needs fixing until it breaks. Anyway, we'll get a version of Searchable out that compiles with 1.8.5, but I strongly vote for rolling back the change on the 1.8.x branch.
[13:09:27 CEST] Jochen: and it is without using default arguments?
[13:10:20 CEST] Jochen: is there a somewhere a link to the source?
[13:21:13 CEST] Jochen: because if no default arguments have been used, then it should have failed before already.
[13:21:28 CEST] Jochen: the default argument usage was therefore a way to bypass this logic
[13:32:01 CEST] Peter Ledbrook: https://github.com/gpc/grails-searchable/blob/master/src/groovy/grails/plugin/searchable/internal/compass/mapping/ClosureSearchableGrailsDomainClassCompassClassMapper.groovy
[13:32:08 CEST] Peter Ledbrook: It's the two init() methods causing the problem.
[13:44:08 CEST] Jochen: so no default parameter usage
[13:44:19 CEST] Jochen: that should not have compiled before
[13:44:41 CEST] Jochen: then I guess we should revert the change for 1.8, since it was more broken than I though
Show
Cedric Champeau added a comment - Patch is reverted in 1.8.6 because it broke some code which previously compiled in Grails. The fix is only on 2.0.x. Here's the discussion: [11:40:55 CEST] Peter Ledbrook: Another one for the Groovy folks: http://pastebin.com/0FUCJzPa [11:41:32 CEST] Peter Ledbrook: Recognise the error? This is from an existing Grails project that compiled fine with Grails 2.0, but fails with 2.0.1 (with Groovy 1.8.5) [11:43:39 CEST] Cédric Champeau: yes, it happens when you have two covariant methods with different modifiers (private/public), Jochen fixed an issue with that recently [11:43:54 CEST] Cédric Champeau: so that's why the warning occurs [11:44:18 CEST] Peter Ledbrook: It's not a warning. It's an error and the compilation fails. Or has it been reverted to a warning for 1.8.6? [11:44:32 CEST] Cédric Champeau: I mean error, there's no warning in Groovy :) [11:45:07 CEST] Peter Ledbrook: Can I say that I think this is a bad change for a patch release? [11:46:26 CEST] Cédric Champeau: You can, but I'm sure Jochen has good arguments :) (That's also a reason for the numbering scheme change, with the last number which should only be used for bugfixes starting from 2.0) [11:47:27 CEST] Peter Ledbrook: Numbering scheme change? You mean from 1.9 -> 2.0? Or something else? [11:47:42 CEST] Peter Ledbrook: Anyway, I believe that there is no good reason to introduce a breaking change into a patch release. [11:47:43 CEST] Cédric Champeau: yes, 1.9 becoming 2.0.0 [11:48:08 CEST] Peter Ledbrook: It happened with Grails and it's a bad idea. We definitely don't want Grails 2.0.1 going out with a breaking change. [11:49:35 CEST] Cédric Champeau: found the jira issue related to this change: https://jira.codehaus.org/browse/GROOVY-5193 [11:49:44 CEST] Cédric Champeau: considered as a bugfix [11:51:40 CEST] Peter Ledbrook: Thanks. But it's still a breaking change and anyone upgrading from Grails 2.0 to 2.0.1 will suddenly hit this compilation error with Searchable. And who knows what other plugins. [11:51:55 CEST] Peter Ledbrook: I'm not defending the code that's not compiling. [11:51:59 CEST] Cédric Champeau: from what I read, it's still possible to remove the fix from 1.8.x branch and keep it in 2.0 [11:52:24 CEST] Peter Ledbrook: That would get my vote :) So is there no way to make it a warning? [11:52:32 CEST] Cédric Champeau: unfortunately, no [11:54:00 CEST] Peter Ledbrook: Ouch. Which means no one knows that code needs fixing until it breaks. Anyway, we'll get a version of Searchable out that compiles with 1.8.5, but I strongly vote for rolling back the change on the 1.8.x branch. [13:09:27 CEST] Jochen: and it is without using default arguments? [13:10:20 CEST] Jochen: is there a somewhere a link to the source? [13:21:13 CEST] Jochen: because if no default arguments have been used, then it should have failed before already. [13:21:28 CEST] Jochen: the default argument usage was therefore a way to bypass this logic [13:32:01 CEST] Peter Ledbrook: https://github.com/gpc/grails-searchable/blob/master/src/groovy/grails/plugin/searchable/internal/compass/mapping/ClosureSearchableGrailsDomainClassCompassClassMapper.groovy [13:32:08 CEST] Peter Ledbrook: It's the two init() methods causing the problem. [13:44:08 CEST] Jochen: so no default parameter usage [13:44:19 CEST] Jochen: that should not have compiled before [13:44:41 CEST] Jochen: then I guess we should revert the change for 1.8, since it was more broken than I though

People

  • Assignee:
    Roshan Dawrani
    Reporter:
    Brian M. Carr
Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: