groovy
  1. groovy
  2. GROOVY-5193

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
        CÚdric 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
        CÚdric 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
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: