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.

Sign up for GitHub

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

Jump to bottom

Assignments to parameters to record compact constructors should not be marked as unused. #6635

Merged
merged 1 commit into from Oct 31, 2023
Merged

Assignments to parameters to record compact constructors should not be marked as unused. #6635

merged 1 commit into from Oct 31, 2023

Conversation

lahodaj
Copy link
Contributor

@lahodaj lahodaj commented Oct 30, 2023 •
edited by mbien

The parameters (and hence the assigned values) are implicitly used at the end of the compact constructors.

Please see:
oracle/javavscode#53

@lahodaj lahodaj added the Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) label Oct 30, 2023
@lahodaj lahodaj added this to the NB21 milestone Oct 30, 2023
@lahodaj lahodaj requested a review from dbalek October 30, 2023 11:05
@lahodaj lahodaj self-assigned this Oct 30, 2023
@lahodaj lahodaj mentioned this pull request Oct 30, 2023
Closed
@mbien
Copy link
Member

mbien commented Oct 30, 2023 •
edited

since this is isolated to hints, we could get this in for rc3 if you want. This would have to be rebased and retargeted to delivery.

regarding records: there are multiple issues coming from the fact that the IDE does not associate the record parameters from the constructor with the final fields of the record. E.g this does also break the rename refactoring, and mark occurrences etc.

I was looking at the unused-hint already before, but was not sure if simply ignoring the params would be the right way to fix it in context of the other problems.

checking if #4257 is fixed by this PR (edit: it is, linking issue)

@mbien mbien added the hints label Oct 30, 2023
@mbien mbien linked an issue Oct 30, 2023 that may be closed by this pull request
Record compact constructor parameters are considered unused #4257
Closed
@mbien
Copy link
Member

mbien commented Oct 30, 2023 •
edited

change causes one failure:

java.lang.AssertionError: 3:8 cannot access Record
class file for java.lang.Record not found
at org.netbeans.modules.java.hints.test.api.HintTest.ensureCompilable(HintTest.java:380)
at org.netbeans.modules.java.hints.test.api.HintTest.run(HintTest.java:459)
at org.netbeans.modules.java.hints.test.api.HintTest.run(HintTest.java:444)
at org.netbeans.modules.java.hints.bugs.UnusedAssignmentOrBranchTest.testRecordCompactConstructor(UnusedAssignmentOrBranchTest.java:226)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at org.netbeans.junit.NbTestCase.access$200(NbTestCase.java:79)
at org.netbeans.junit.NbTestCase$2.doSomething(NbTestCase.java:484)
at org.netbeans.junit.NbTestCase$1Guard.run(NbTestCase.java:405)
at java.base/java.lang.Thread.run(Thread.java:829)

noticed also this second exception in the log (edit: exception is also on the master branch):

    [junit] java.lang.AssertionError: class def not found: Base() in base.Base
    [junit] 	at com.sun.tools.javac.util.Assert.error(Assert.java:162)
    [junit] 	at com.sun.tools.javac.comp.Lower.makeAccessible(Lower.java:1395)
    [junit] 	at com.sun.tools.javac.comp.Lower.translateTopLevelClass(Lower.java:4295)
    [junit] 	at com.sun.tools.javac.main.JavaCompiler.desugar(JavaCompiler.java:1654)
    [junit] 	at org.netbeans.lib.nbjavac.services.NBJavaCompiler.desugar(NBJavaCompiler.java:89)
    [junit] 	at com.sun.tools.javac.main.JavaCompiler.desugar(JavaCompiler.java:1468)
    [junit] 	at com.sun.tools.javac.api.JavacTaskImpl$2.process(JavacTaskImpl.java:475)
    [junit] 	at com.sun.tools.javac.api.JavacTaskImpl$Filter.run(JavacTaskImpl.java:519)
    [junit] 	at com.sun.tools.javac.api.JavacTaskImpl.generate(JavacTaskImpl.java:478)
    [junit] 	at org.netbeans.modules.java.source.indexing.VanillaCompileWorker$2.run(VanillaCompileWorker.java:369)
    [junit] 	at org.netbeans.modules.java.source.parsing.FileManagerTransaction.runConcurrent(FileManagerTransaction.java:180)
    [junit] 	at org.netbeans.modules.java.source.indexing.VanillaCompileWorker.compile(VanillaCompileWorker.java:356)
    [junit] 	at org.netbeans.modules.java.source.indexing.JavaCustomIndexer.index(JavaCustomIndexer.java:360)
    [junit] 	at org.netbeans.modules.parsing.spi.indexing.Indexable$MyAccessor$2.run(Indexable.java:138)
    [junit] 	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater.runIndexer(RepositoryUpdater.java:274)
    [junit] 	at org.netbeans.modules.parsing.spi.indexing.Indexable$MyAccessor.index(Indexable.java:136)
    [junit] 	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$Work.doIndex(RepositoryUpdater.java:2749)
    [junit] 	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$Work.lambda$index$0(RepositoryUpdater.java:2626)
    [junit] 	at org.netbeans.modules.parsing.impl.indexing.errors.TaskCache.refreshTransaction(TaskCache.java:540)
    [junit] 	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$Work.index(RepositoryUpdater.java:2625)
    [junit] 	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$Work.lambda$scanFiles$2(RepositoryUpdater.java:3332)
    [junit] 	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater.lambda$runInContext$4(RepositoryUpdater.java:2119)
    [junit] 	at org.openide.util.lookup.Lookups.executeWith(Lookups.java:288)
    [junit] 	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater.runInContext(RepositoryUpdater.java:2117)
    [junit] 	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater.runInContext(RepositoryUpdater.java:2098)
    [junit] 	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater.access$1400(RepositoryUpdater.java:135)
    [junit] 	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$Work.scanFiles(RepositoryUpdater.java:3290)
    [junit] 	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$FileListWork.getDone(RepositoryUpdater.java:3832)
    [junit] 	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$Work.doTheWork(RepositoryUpdater.java:3452)
    [junit] 	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$Task._run(RepositoryUpdater.java:6197)
    [junit] 	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$Task.access$3400(RepositoryUpdater.java:5855)
    [junit] 	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$Task$2.lambda$call$0(RepositoryUpdater.java:6116)
    [junit] 	at org.openide.util.lookup.Lookups.executeWith(Lookups.java:288)
    [junit] 	at org.netbeans.modules.parsing.impl.RunWhenScanFinishedSupport.performScan(RunWhenScanFinishedSupport.java:83)
    [junit] 	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$Task$2.call(RepositoryUpdater.java:6116)
    [junit] 	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$Task$2.call(RepositoryUpdater.java:6112)
    [junit] 	at org.netbeans.modules.masterfs.filebasedfs.utils.FileChangedManager.priorityIO(FileChangedManager.java:153)
    [junit] 	at org.netbeans.modules.masterfs.providers.ProvidedExtensions.priorityIO(ProvidedExtensions.java:335)
    [junit] 	at org.netbeans.modules.parsing.nb.DataObjectEnvFactory.runPriorityIO(DataObjectEnvFactory.java:118)
    [junit] 	at org.netbeans.modules.parsing.impl.Utilities.runPriorityIO(Utilities.java:67)
    [junit] 	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$Task.run(RepositoryUpdater.java:6112)
    [junit] 	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
    [junit] 	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
    [junit] 	at org.openide.util.RequestProcessor$Task.run(RequestProcessor.java:1420)
    [junit] 	at org.netbeans.modules.openide.util.GlobalLookup.execute(GlobalLookup.java:45)
    [junit] 	at org.openide.util.lookup.Lookups.executeWith(Lookups.java:287)
    [junit] 	at org.openide.util.RequestProcessor$Processor.run(RequestProcessor.java:2035)

@lahodaj lahodaj changed the base branch from master to delivery October 30, 2023 14:09
@lahodaj
Copy link
Contributor Author

lahodaj commented Oct 30, 2023

Right, the test must be disabled when j.l.Record is not present in the platform (that is what we typically do for other tests like this).

The exception in the log seems unrelated to this patch, so something to investigate independently.

Rebased to delivery.

@lahodaj
Copy link
Contributor Author

lahodaj commented Oct 30, 2023

regarding records: there are multiple issues coming from the fact that the IDE does not associate the record parameters from the constructor with the final fields of the record. E.g this does also break the rename refactoring, and mark occurrences etc.

I was looking at the unused-hint already before, but was not sure if simply ignoring the params would be the right way to fix it in context of the other problems.

I am afraid that overall, we may need to tweak the feature to understand the records properly - there's probably no simple solution that would solve everything in one go.

@mbien
Copy link
Member

mbien commented Oct 30, 2023 •
edited

Right, the test must be disabled when j.l.Record is not present in the platform (that is what we typically do for other tests like this).

true! If there are many tests with JDK 17+ runtime requirements, we should consider a [11, 21] matrix. If its just a few, we could isolate them and run them separately in a JDK 17 or 21 step. (NB 22 will bump the testing baseline to JDK 17).

@mbien mbien modified the milestones: NB21, NB20 Oct 30, 2023
mbien
mbien approved these changes Oct 31, 2023
View reviewed changes
Copy link
Member

@mbien mbien left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes make sense to me. Tests are green, tested it manually too and the compact constructor params are no longer seen as unused.

@neilcsmith-net neilcsmith-net changed the title [JAVAVSCODE-53] Assignments to parameters to record compact constructors should not be marked as unused. Assignments to parameters to record compact constructors should not be marked as unused. Oct 31, 2023
@neilcsmith-net
Copy link
Member

Thanks, merging. I've edited the title as this shows in our release notes. Linking to third-party issues is fine, but as with Payara FISH labels, let's leave them out of titles or open a related issue here to reference.

@neilcsmith-net neilcsmith-net merged commit 68bb0e8 into apache:delivery Oct 31, 2023
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbien mbien mbien approved these changes

@dbalek dbalek Awaiting requested review from dbalek

Assignees

@lahodaj lahodaj

Labels
hints Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form)
Projects
None yet
Milestone
NB20
Development

Successfully merging this pull request may close these issues.

Record compact constructor parameters are considered unused
3 participants
@lahodaj @mbien @neilcsmith-net