Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unexpected compile error for sealed-interface without permits inside record #1641

Closed
lukeu opened this issue Nov 28, 2023 · 7 comments · Fixed by #1713
Closed

Unexpected compile error for sealed-interface without permits inside record #1641

lukeu opened this issue Nov 28, 2023 · 7 comments · Fixed by #1713
Assignees

Comments

@lukeu
Copy link

lukeu commented Nov 28, 2023

Given the following, which javac accepts...

	record InterfaceInRecord() {
	    sealed interface I {
	        enum Empty implements I {
	            INSTANCE;
	        }
	        record Single(double value) implements I {
	        }
	    }
	}

... Eclipse issues 2 compiler errors on the lines containing implements I

Here are 2 modifications that (independently) resolve the errors:

  • Change the outer record into a class; or
  • Following sealed interface I insert permits I.Empty, I.Single (A useful workaround)

Based on this evidence (nb: without reading the language spec) I'm inclined to think the Eclipse compiler is wrong in this case.

Tested using:

  • Eclipse Version: 2023-12 (4.30 RC2) - Build id: I20231123-1700
  • Running on Java Temurin: jdk-21.0.0.35-hotspot
  • Trying both "Compiler Compliance Level" 17 and 21
@lukeu lukeu changed the title Compile error given incorrectly on inner sealed-interface without a permits clause Unjustified compile error for inner sealed-interface without permits inside record Nov 28, 2023
@lukeu lukeu changed the title Unjustified compile error for inner sealed-interface without permits inside record Unjustified compile error for sealed-interface without permits inside record Nov 28, 2023
@lukeu
Copy link
Author

lukeu commented Dec 2, 2023

@jarthana, @mpalat FYI I've done some digging into the code, and this issue seems related to the functionality added here:

Bug 564498 - [15] JEP 360 error flagged for implicit permits in inner classes
Change-Id: Id649441c9c4cd16cdf227e50c4a20b8ae5751335
Signed-off-by: Jay Arthanareeswaran

There was a further small change added into ClassScope$connectImplicitPermittedTypes later:

-  if (sourceType.id == TypeIds.T_JavaLangObject || sourceType.isEnum()) // already handled
+  if (sourceType.id == TypeIds.T_JavaLangObject || sourceType.isEnum() || sourceType.isRecord()) // already handled

From within this commit:

Bug 565388 - [15] sealed error to be changed to be more meaningful
Change-Id: Ie38d894e11f5ae57cbf6bb3df0c873ca92561ddb
Signed-off-by: Manoj Palat

Which seems to make this method return early, thus not populating the "permitted types" for the inner-most types in this scenario.

Here's a more minimal test-case, which is easier to debug:

	record InterfaceInRecord() { 
	    sealed interface I  {
	        final class C implements I {
	        }
	    }
	}

The innermost class InterfaceInRecord$I$C incorrectly ends up with a blank array of permitted types.

Removing the isRecord() early-exit appears to make the issue go away. However I am out of my depth as to whether that clause is necessary in some other situations.

@lukeu lukeu changed the title Unjustified compile error for sealed-interface without permits inside record Unexpected compile error for sealed-interface without permits inside record Dec 3, 2023
@mpalat
Copy link
Contributor

mpalat commented Dec 5, 2023

Thanks @lukeu for providing the test case and investigating further - will take a look

@mpalat mpalat mentioned this issue Dec 8, 2023
3 tasks
mpalat added a commit that referenced this issue Dec 8, 2023
@lukeu
Copy link
Author

lukeu commented Dec 8, 2023

Hi, thanks for looking into this. I played around with the fix and I believe similar bugs remain when the sealed interface is inside an enum rather than a record. Here is another test case:

enum InterfaceInEnum {
    INSTANCE;
    sealed interface I  {
        final class C implements I {}
    }
    final class D implements I {}
}

Similarly, Eclipse reports 2x compiler errors here, whereas javac reports none.

However, when attempting to test the workaround, a crash occurs. Specifically, this is thrown after typing: permits I.

java.lang.NullPointerException: Cannot read the array length because "superPermittedTypes" is null
at org.eclipse.jdt.internal.compiler.lookup.SourceTypeBinding.checkPermitsAndAdd(SourceTypeBinding.java:1342)
at org.eclipse.jdt.internal.compiler.lookup.SourceTypeBinding.checkPermitsInType(SourceTypeBinding.java:1203)
at org.eclipse.jdt.internal.compiler.lookup.SourceTypeBinding.checkPermitsInType(SourceTypeBinding.java:1210)
at org.eclipse.jdt.internal.compiler.lookup.SourceTypeBinding.checkPermitsInType(SourceTypeBinding.java:1210)
at org.eclipse.jdt.internal.compiler.lookup.SourceTypeBinding.faultInTypesForFieldsAndMethods(SourceTypeBinding.java:1112)
at org.eclipse.jdt.internal.compiler.lookup.CompilationUnitScope.faultInTypes(CompilationUnitScope.java:603)
at org.eclipse.jdt.core.dom.CompilationUnitResolver.resolve(CompilationUnitResolver.java:1314)
at org.eclipse.jdt.core.dom.CompilationUnitResolver.resolve(CompilationUnitResolver.java:790)
at org.eclipse.jdt.core.dom.ASTParser.internalCreateASTCached(ASTParser.java:1242)
at org.eclipse.jdt.core.dom.ASTParser.lambda$0(ASTParser.java:1120)
at org.eclipse.jdt.internal.core.JavaModelManager.cacheZipFiles(JavaModelManager.java:5812)
at org.eclipse.jdt.core.dom.ASTParser.internalCreateAST(ASTParser.java:1120)
at org.eclipse.jdt.core.dom.ASTParser.createAST(ASTParser.java:868)
at org.eclipse.jdt.core.manipulation.CoreASTProvider$1.run(CoreASTProvider.java:294)
at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:47)
at org.eclipse.jdt.core.manipulation.CoreASTProvider.createAST(CoreASTProvider.java:286)
at org.eclipse.jdt.core.manipulation.CoreASTProvider.getAST(CoreASTProvider.java:199)
at org.eclipse.jdt.core.manipulation.CoreASTProvider.getAST(CoreASTProvider.java:189)
at org.eclipse.jdt.core.manipulation.SharedASTProviderCore.getAST(SharedASTProviderCore.java:138)

This also appears to be to do with permitted types in the AST, so maybe another side of the same coin?


Adding a further line creates a different variant of the test case above. (With this it does not seem to trigger the crash):

    final class E implements InterfaceInEnum.I {}
  • Observed: 3 compile errors
  • Expected: No compile errors - the same as if the user had explicitly written sealed interface I permits I.C, D, E

@mpalat
Copy link
Contributor

mpalat commented Dec 11, 2023

Thanks @lukeu for the variants - will check. should have taken care of enums as well..
btw, didn't understand the workaround part of "However, when attempting to test the workaround, a crash occurs. ", which workaround are you referring to? I am not getting this stacktrace

@mpalat mpalat reopened this Dec 11, 2023
@lukeu
Copy link
Author

lukeu commented Dec 11, 2023

which workaround are you referring to?

I mean that after: sealed interface I I began typing permits I.C, D, E and as soon as I typed the '.' I saw that stack-trace.

As for my environment, I am debugging Eclipse from Eclipse as described here.

@mpalat
Copy link
Contributor

mpalat commented Dec 11, 2023

I mean that after: sealed interface I I began typing permits I.C, D, E and as soon as I typed the '.' I saw that stack-trace.

Ah, I see. Thanks for the immediate response. able to reproduce this - the pr fixes it though its a different issue. -will see whether a fix can be piggybacked with this pr. Thanks

@mpalat mpalat mentioned this issue Dec 13, 2023
3 tasks
jarthana pushed a commit to jarthana/eclipse.jdt.core that referenced this issue Dec 13, 2023
noopur2507 pushed a commit that referenced this issue Dec 15, 2023
@mpalat
Copy link
Contributor

mpalat commented Dec 19, 2023

I mean that after: sealed interface I I began typing permits I.C, D, E and as soon as I typed the '.' I saw that stack-trace.

Ah, I see. Thanks for the immediate response. able to reproduce this - the pr fixes it though its a different issue. -will see whether a fix can be piggybacked with this pr. Thanks

@lukeu I've merged the fix for enums also last week via #1739 - Please let me know whether you see any further issues in your setup. closing this now.

@mpalat mpalat closed this as completed Dec 19, 2023
mpalat added a commit that referenced this issue Dec 19, 2023
* configure default output.. = bin/

* JavaSearchScope: improve encloses() performance  #474

* [test] remove outdated latestBREE project

* fix some ecj markers

Especially after moving files to compiler.batch - which has no resource
warnings - the @SuppressWarnings("resource") is not used - leading to a
marker

* Single async "Synchronizing projects" Job #419

Scheduling multiple times "while the job is running, the job will still
only be rescheduled once" (javadoc) so there will be always only a
single job running.
Using a Set prevents touching projects multiple times.

#419

Manually tested by changing Compiler Building options "Circular
dependencies", which triggers a new build.

* [test] fix AbstractJavaModelTests #1333

avoid asynchronous refresh. Implementation taken from
org.eclipse.core.tests.resources.refresh.RefreshProviderTest.joinAutoRefreshJobs()

#1333

* TestVerifier: never wait 100ms

improve test time, accurate timeout

* [performance] ClassFileReader: use already open Zip File

instead of creating new ZipFile instance.

also:
* use Files.readAllBytes
* removed unused code
* faster toUri avoiding isDirectory() check for the JARs which are known
to be no directory

improves performance of read() on windows by factor 2
tested with java reference search to java.lang.Object

* Stop skipping compare-with-baseline for jdt.annotation v2

One less thing that has to be manually tracked and done.

* Version bump for jdt.annotation

Pointed by
https://download.eclipse.org/eclipse/downloads/drops4/I20231130-0020/buildlogs/reporeports/reports/versionChecks.html

* Fix github urls in NOTICE file

* Use try-with-resource and enable warning if not

org.eclipse.jdt.core.compiler.problem.explicitlyClosedAutoCloseable=warning

StringWriter does not need flush() or close() and can not throw
IOException.

Tests excluded.

To get rid of boiler plate code.

* version bumps

* [21] JEP 430 String Templates (#1513)

Grammar, compiler AST, resolution and code generation changes

* Javadoc: fix unclosed <code>

* break from loop within labeled block causes loss of nullness info (#1660)

fixes #1659

* Fixes incorrect Javadoc after StringBuffer to StringBuilder change

* NPE in ASTRewriteFlattener as return value of GuardedPattern.getPattern() is null  (#1647)

NPE in ASTRewriteFlattener as return value of
GuardedPattern.getPattern() is null

* Adding pomless build to JDT core

This enables pomless builds for JDT coreand removes the simple pom
files.

Future commits can reduce the usage of pom files further. This might
require enhancements in pomless builds to specify the test classes and
suites eclipse-tycho/tycho#3105

* Using Simplify lambda expression and method reference syntax cleanup on
core

Using the JDT UI "Simplify lambda expression and method reference
syntax" clean-up on jdt.core.

* Internal compiler error: ArrayIndexOutOfBoundsException in latest i build (#1664)


fixes #1661

* Using Simplify lambda expression and method reference syntax cleanup on
all plug-ins except core

Using the JDT UI "Simplify lambda expression and method reference
syntax" clean-up on all plug-ins except jdt.core.

* Using short-circuit in IncrementalImageBuilder

* Re-normalize line-endings in git of all files to Linux style ("\n")

Some files were checked-in into git having windows style line
endings (\r\n). This is in general not wanted because it can cause
modified files without any difference in git-staging on Windows if
auto-crlf is enabled.

To re-normalize line endings of all files use the following command
(including dot):

git add --renormalize .

* Bump bundle dependencies to trigger a rebuild / fix SDK build error

The change in bytecode of FullSourceWorkspaceBuildTests is due the
constant value change of
o.e.jdt.internal.compiler.parser.TerminalTokens.TokenNameEOF coming
from #1513.

Fixes eclipse-platform/eclipse.platform.releng.aggregator#1617

* False positive "Dead code" compiler error reported on org.eclipse.pde.internal.core.util.PDEJavaHelper.getExternalPackageFragment(String, String) (#1671)

fixes #1667

* Use diamond operator in jdt.core repo

Using the JDT UI clean-up, this removes the redundant type information.

Also activating
org.eclipse.jdt.core.compiler.problem.redundantSpecificationOfTypeArguments=warning
The rest of the changes in the prefs file were done by the tooling, not
by
selecting anther value.

* Improve constructor completions inside method invocations Fix #1587 (#1588)

* Try-with-resource clean-up in JDT core

Using the JDT UI try-with-resource clean-up on core.
Also manually inlining the declaration of multiple places into the try()
clause.

* Make inner classes static where possible in JDT core

Running the JDT performnace clean-up "Make inner classes static where
possible" in JDT core

* Use Use lazy logical operator (&& and ||)

Running the performance clean-up "Use lazy logical operator (&& and ||)"
on JDT core

* Using Integer.toString directly in Disassembler

It is also a JDT UI performance clean-up but this clean-up found only
one occurrence.

* use Path.of() to avoid 'Potential resource leak' warnings

* @NonNullByDefault does not work for type arguments of a local type (#1694)

fixes #1693

* ClassCastException during code completion on Annotation (#1696)

fixes #1440

Also fixes noise from #1662

* CompilationUnitResolver: Name the CU that causes Exception (#1690)

for example during Cleanup
eclipse-jdt/eclipse.jdt.ui#950

Co-authored-by: Jörg Kubitz <jkubitz-eclipse@gmx.de>

* Code selection support for String template expressions (#1699)

Fixes  #1685

* Report error if string template is used without preview option enabled (#1697)

Improves the fix for #544

* ECJ crashes when an embedded expression contains broken code (#1702)

Set haltOnSyntaxError when parsing for embedded expressions. Also using the correct delimiters for text blocks in printExpression() methods.

* Selection model tests for string templates (#1704)

* tests: enable discouragedReference=warning, declare x-friends

to get rid of warnings during build (does not respect the jdt
preferences)

* Javadoc format fixes

Contributes to
eclipse-platform/eclipse.platform.releng.aggregator#1531

* Performance: Add public API for Batch Reads in UI - closes #1614

During Batches:
* cache Zip Files
* enable JavaModelManager.temporaryCache

Also:
* uses Batch Reads during some core actions that benefit from it.
* adds trace logging when caching is missing.

#1614

* Javadoc format fixes (part 2)

Contributes to
eclipse-platform/eclipse.platform.releng.aggregator#1531.
It's a pity that this takes multiple cycles but fixing one thing from
the log uncovers the next.

* ECJ 3.36.0 regression: The type 'E extends java.lang.Exception' is not a valid substitute for the type parameter 'E extends java.lang.@nonnull Exception' (#1708)

fixes #1691

+ also slightly updates NullAnnotationTests18 as NullAnnotationTests21

* Implement support for code completion inside embedded expression of (only) string templates (not text block templates) (#1712)

* Implement support for code completion inside embedded expression of
(only) string templates (not text block templates)

* Fixes #1711

* Fixes #1641 (#1713)

* Add new testcase #1701 (#1705)

* Content assist does not propose overrides in records (#1718)

* Fixes #1095

* JavaModelManager: lazy initialize TouchJob #1720

#1720

* Run the JavaCoreStandaloneTest in the build

#1720

* Bogus error about return expression involving pattern matching (#1731)

Fixes #1726

* Compiler fails to recognize an exhaustive switch (#1733)

*  Fixes #1725

* [21] Processed string templates falsely contain backslash characters (#1730)

#1719

* 1641_enum_further_fixes (#1739)

* Fix one too many pops arising from string concat invoke dynamic (#1740)

* Fix one too many pops arising from string concat invoke dynamic

* Fixes #1394

* Wrong placement of exception range closure results in AIOOB (#1744)

Fixes #1686

* Improper warning - Potential null pointer access (#1469)

Fixes #1461
Sets the flow info reach mode to FlowInfo.UNREACHABLE_BY_NULLANALYSIS
after 'Object o = null;if (Objects.isNull(o)) return;', 'Object o = "";if (Objects.nonNull(o)) return;

Signed-off-by: Snjezana Peco <snjezana.peco@redhat.com>

* Touch bundles affected by the changed ecj version

See #1394
See eclipse-platform/eclipse.platform.releng.aggregator#1659

* Upload eventfile and unit test results

* 2023-06->2023-09 Seems to have broke dependency graph management in our project (#1698)

* Add test for reproducing #1654
* Fix to make that test pass

---------

Co-authored-by: Stephan Herrmann <stephan.herrmann@berlin.de>

* [memory] SoftReference for ResourceCompilationUnit.contents #1743

Ability to reduce memory during searches that find many files

#1743

* deduplicate "eclipse" #1743

CharDeduplication was not designed to deduplicate tokens with length 7+
which could lead to high memory consumption. With this change tokens of
all sizes can be deduplicated.

#1743

A benchmark implemented in CharDeduplicationTest.main(String[]) shows
the new deduplication is performed at similar speed (.21s instead of
.16s) but deduplicates much more tokens (99% instead of 36%).

* 1703.constant definitions (#1756)

* Fixes [21] AIOOB at switchStatement TNode.addPattern  (#1757)

org.eclipse.jdt.internal.compiler.ast.SwitchStatement $TNode.addPattern

---------

Signed-off-by: Snjezana Peco <snjezana.peco@redhat.com>
Co-authored-by: Jörg Kubitz <jkubitz-eclipse@gmx.de>
Co-authored-by: Eric Milles <eric.milles@thomsonreuters.com>
Co-authored-by: Александър Куртаков <akurtakov@gmail.com>
Co-authored-by: Jay Arthanareeswaran <jarthana@in.ibm.com>
Co-authored-by: Stephan Herrmann <stephan.herrmann@berlin.de>
Co-authored-by: Lars Vogel <Lars.Vogel@vogella.com>
Co-authored-by: Suby S Surendran <suby.surendran@ibm.com>
Co-authored-by: Hannes Wellmann <wellmann.hannes1@gmx.net>
Co-authored-by: Andrey Loskutov <loskutov@gmx.de>
Co-authored-by: Gayan Perera <gayanper@gmail.com>
Co-authored-by: Jörg Kubitz <51790620+jukzi@users.noreply.github.com>
Co-authored-by: Srikanth Sankaran <131454720+srikanth-sankaran@users.noreply.github.com>
Co-authored-by: Ed Merks <ed.merks@gmail.com>
Co-authored-by: Snjeza <snjezana.peco@redhat.com>
Co-authored-by: Christoph Läubrich <laeubi@laeubi-soft.de>
Co-authored-by: Michael Haubenwallner <michael.haubenwallner@ssi-schaefer.com>
rgrunber pushed a commit to eclipse-jdtls/eclipse-jdt-core-incubator that referenced this issue Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants