Community
Participate
Working Groups
Created attachment 262604 [details] Class to reproduce issue with Seeing the following messages when compiling a source file generated by the protoc-jar-maven-plugin maven plugin. The generated source file does compile fine when running maven from command line. Description Resource Path Location Type The return types are incompatible for the inherited methods MessageOrBuilder.getAllFields(), GeneratedMessage.getAllFields() EventInfo.java /utils/target-ide/generated-sources/protbuf/com/mylaps/taguse/proto line 85 Java Problem Description Resource Path Location Type The return types are incompatible for the inherited methods MessageOrBuilder.getAllFields(), GeneratedMessage.Builder<EventInfo.EventInfoStorage.Builder>.getAllFields() EventInfo.java /utils/target-ide/generated-sources/protbuf/com/mylaps/taguse/proto line 507 Java Problem We didn't see this issue when running a previous release candidate, only when upgrading to the final Neon release. We can reproduce on OSX and Windows, both with Java 8. A generated protocol buffer file is attached that can reproduce the issue, steps to reproduce are: 1. Create a new Java 1.8 project and place the file in the com.mylaps.taguse.proto package 2. Add the protobuf 2.6.1 jar to the project ( http://search.maven.org/remotecontent?filepath=com/google/protobuf/protobuf-java/2.6.1/protobuf-java-2.6.1.jar ) 3. Compile, the two errors are shown.
This is a regression introduced in 4.6RC1
The errors are reported since the fix for Bug 492327. I have no idea yet, what might be the connection.
I'll extract a test case.
New Gerrit change created: https://git.eclipse.org/r/75882
(In reply to Eclipse Genie from comment #4) > New Gerrit change created: https://git.eclipse.org/r/75882 This change just contains a failing test case (in NullTypeAnnotationTest) The reported bug does not depend on annotation based null analysis being enabled, but I forgot to turn it off while reducing the code. I'll try to make another case that triggers the bug without it enabled some other day.
Not sure if this is helpful, but in order to avoid the issue I tried rolling back to a previous release (M6, M5, ..., M1) but still saw the same behavior in all of them. Which is odd since I had been running one of those releases before upgrading to the general release yesterday, and hadn't seen the issue previously. What I found would work was if I installed a previous release, but did not install the m2e connector recommended to fix the following error: Description Resource Path Location Type Plugin execution not covered by lifecycle configuration: org.codehaus.mojo:build-helper-maven-plugin:1.8:add-source (execution: add-source, phase: generate-sources) pom.xml /application line 6 Maven Project Build Lifecycle Mapping Problem This seems odd since I still had the source file which caused the issue in the build path of my project - it isn't until I install: m2e connector for build-helper-maven-plugin 0.15.0.201207090124 that I see the errors for that source file. It also appears that in a new (non-maven) project I'd created to reproduce the issue, I also don't see the errors until that m2e connector is added.
(In reply to Eclipse Genie from comment #4) > New Gerrit change created: https://git.eclipse.org/r/75882 Thanks for the test! The patch shows the entire test file as changed. This looks like a problem of mismatching line ends. IIRC this file was accidentally pushed with \r\n some time ago. We should make up our minds whether: - we keep it in this state to reduce noise in the history, or - make an otherwise empty commit to convert back to \n I'm slightly leaning towards the latter, because that will also make pasting test code into string quotes nicer, with only \n, where currently it inserts \r\n I wouldn't mix a real commit with that conversion, though.
(In reply to Till Brychcy from comment #5) > The reported bug does not depend on annotation based null analysis being > enabled, but I forgot to turn it off while reducing the code. I'll try to > make another case that triggers the bug without it enabled some other day. I just noted: If annotations based null analysis is off, the error appears only in the editor, not in the problems view.
(In reply to Stephan Herrmann from comment #7) > The patch shows the entire test file as changed. This looks like a problem > of mismatching line ends. IIRC this file was accidentally pushed with \r\n > some time ago. Strange, I have added tests to this file previously and this has not happened. Maybe something has been changed in egit? > > We should make up our minds whether: > - we keep it in this state to reduce noise in the history, or Shouldn't matter for Tests. > - make an otherwise empty commit to convert back to \n > > I'm slightly leaning towards the latter, because that will also make pasting > test code into string quotes nicer, with only \n, where currently it inserts > \r\n I agree. I've looked for other such cases and found that ASTConverter18Test and FormatterBugsTests also have CRLF line endings.
(In reply to Till Brychcy from comment #8) > (In reply to Till Brychcy from comment #5) > > The reported bug does not depend on annotation based null analysis being > > enabled, but I forgot to turn it off while reducing the code. I'll try to > > make another case that triggers the bug without it enabled some other day. > > I just noted: If annotations based null analysis is off, the error appears > only in the editor, not in the problems view. And this happens, because in CompilationUnitProblemFinder.getCompilerOptions, compilerOptions.storeAnnotations is set to true (creatingAST is true), so the AnnotatedTypeSystem is used even if annotations based null analysis is off. So the bug does not appear if the "base" TypeSystem is used and no other test case is needed.
This issue is observed in the Neon official release. My dev environment is Eclipse 4.6 SDK for win32_64, windows 10 Prof, jdk8u91 x64. Just create an empty proto message, generate the java binding using protoc, then setup the project in Eclipse. The source code compiled OK outside using javac from command line, but shows two error marks in the java source editor saying "The return types are incompatible for the inherited methods MessageOrBuilder.getAllFields(), GeneratedMessage.getAllFields()". A bit odd that there is no error in the Problems view, and no decorator in the project explorer. We have a project has hundreds of protobuf messages. The same code shows no error in Eclipse 4.4 with the same jdk. But with Neon, we can not build our plugins using the PDE build scripts because of this. A simple project attached for further investigation. Thanks!
Created attachment 262697 [details] Sample eclipse project shows the issue
The fix for bug 492327 added a raw conversion because this was done before the fix for bug 473713 by having the "true" parameter in type = BinaryTypeBinding.resolveType(type, this.environment, true). But previously, this was only done for nested classes. Patch set 2 refines this, so the explicit raw conversion is only done for nested classes. This restores the previous behavior more precisely and fixes the problem of this bug, but this distinction currently doesn't really make sense to me.
> This restores the previous behavior more precisely and fixes the problem of > this bug, but this distinction currently doesn't really make sense to me. Here is my explanation: If no generics are involved at all, the raw conversion has no effect. Parameterized nested types are already resolved in LookupEnvironment.getTypeFromTypeSignature and do not reach this point at all. But static unparameterized types nested in a parametrized type are currently represented as ParameterizedTypeBinding (see bug 460491), and for those the raw conversion simply has the right effect of turning the BinaryTypeBinding exactly into the corresponding ParameterizedTypeBinding required for this case (it is not known until the URB is resolved, that the enclosing type is parametrized, so this cannot be prepared for earlier). For toplevel types, the type signature already tells if a PTB needs to be created or a raw type is needed, and no extra raw conversion must be done in getUnannotatedType.
@Stephan, I guess it would be a good idea to either change the line endings in NTAT and the other files now and push this change immediately to the maintenance branch, too - or leave things as they are. What do you think? Or should somebody else be consulted first?
(In reply to Till Brychcy from comment #15) > @Stephan, I guess it would be a good idea to either change the line endings > in NTAT and the other files now and push this change immediately to the > maintenance branch, too - or leave things as they are. What do you think? > Or should somebody else be consulted first? I think it's good to make the change in master now, in a commit of its own. Wrt the maintentance branch let's wait until we have an approved back port request affecting this file. I'll try to look into bug 460491 soonish ...
(In reply to Stephan Herrmann from comment #16) > (In reply to Till Brychcy from comment #15) > > @Stephan, I guess it would be a good idea to either change the line endings > > in NTAT and the other files now and push this change immediately to the > > maintenance branch, too - or leave things as they are. What do you think? > > Or should somebody else be consulted first? > > I think it's good to make the change in master now, in a commit of its own. > Wrt the maintentance branch let's wait until we have an approved back port > request affecting this file. OK, i've created bug 497419 for the line endings changes. > > > I'll try to look into bug 460491 soonish ... Do you agree that I push my patch (to TypeSystem.java) for this bug into the master first (after the line endings change)? It is simple, low-risk and might by be a candidate for 4.6.1
Just ran into the same problem with Protobuf-generated code. Failing to compile valid code is a significant and annoying regression - this should go into a maintenance release ASAP.
Gerrit change https://git.eclipse.org/r/75882 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=08c091450d477172212fb94d4161d048b450d825
(In reply to Eclipse Genie from comment #19) > Gerrit change https://git.eclipse.org/r/75882 was merged to [master]. > Commit: > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/ > ?id=08c091450d477172212fb94d4161d048b450d825 Released for 4.7M1
@Till, while I'm trying to understand the connection between this bug and bug 460491, can you tell me if non-static member types still need some of the resolving / conversion discussed here? I.e., when bug 460491 takes care of static member types, does this indeed simplify the situation in TypeSystem?
(In reply to Stephan Herrmann from comment #21) > @Till, while I'm trying to understand the connection between this bug and > bug 460491, can you tell me if non-static member types still need some of > the resolving / conversion discussed here? I.e., when bug 460491 takes care > of static member types, does this indeed simplify the situation in > TypeSystem? If the type signature has type parameters, no URBs are created for nested types (they are immediately resolved in LookupEnvironment). So yes, with your patches the corresponding if()-block in LE.getUnnannotatedType can be simplified to: if (type.isUnresolvedType()) { urb = (UnresolvedReferenceBinding) type; ReferenceBinding resolvedType = urb.resolvedType; if (resolvedType != null) { type = resolvedType; } }
*** Bug 498057 has been marked as a duplicate of this bug. ***
*** Bug 498486 has been marked as a duplicate of this bug. ***
Verified for 4.7 M1 with build I20160802-0800 +1 for 4.6.1
@Till, with all that we have learned during the last weeks, is http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=08c091450d477172212fb94d4161d048b450d825 still the best thing we can do for 4.6.1 (given that bug 460491 will not be back ported)? Please prepare a gerrit for the maintenance branch, TIA.
New Gerrit change created: https://git.eclipse.org/r/78710
(In reply to Stephan Herrmann from comment #26) > @Till, with all that we have learned during the last weeks, is > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/ > ?id=08c091450d477172212fb94d4161d048b450d825 still the best thing we can do > for 4.6.1 (given that bug 460491 will not be back ported)? Yes! > > Please prepare a gerrit for the maintenance branch, TIA. (In reply to Eclipse Genie from comment #27) > New Gerrit change created: https://git.eclipse.org/r/78710 Done.
Gerrit change https://git.eclipse.org/r/78710 was merged to [R4_6_maintenance]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=da6c68ad40d33db9951cf2a69dd01a3fe6a72b89
(In reply to Eclipse Genie from comment #29) > Gerrit change https://git.eclipse.org/r/78710 was merged to > [R4_6_maintenance]. > Commit: > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=da6c68ad40d33db9951cf2a69dd01a3fe6a72b89 > Reviewed positively and released for 4.6.1. Thanks!
*** Bug 499344 has been marked as a duplicate of this bug. ***
Verified for 4.6.1 with build M20160817-0420
