search

eclipse-jdt / eclipse.jdt.core

Eclipse Public License 2.0
164 stars 130 forks source link

Bugfix for #985 (ECJ throws false-positive "Package collides with type" on Windows) #3057

Open nettozahler opened 1 month ago

nettozahler commented 1 month ago

What it does

Fixes #985

How to test

Use the self-contained example from the bug report to reproduce the issue. But change the setup to run it against a current version of ECJ which contains this bugfix (the original reproducer uses ecj-3.33.0.jar). The bug does not occur with this bugfix.

Review hints

To simplify the review process, there are several distinct commits to improve the code style. These are commentd with "No runtime changes". The bugfix is contained in the last commit. After review and before merge the commits can be squashed to a single one.

Author checklist

stephan-herrmann commented 1 month ago

Thanks for the PR

To simplify the review process, there are several distinct commits to improve the code style. These are commentd with "No runtime changes". The bugfix is contained in the last commit. After review and before merge the commits can be squashed to a single one.

I'm glad you separated those different concerns, but frankly I can't see (a) that the changes of style have any relation to the actual bugfix, nor (b) that those are objectively corrections or otherwise improvements. E.g., adding javadoc to internal classes without providing additional information is definitely a non-goal. The same for shortening identifiers or reducing LOC (whereas simplification could be a goal, if all agree that the result is simpler).

Note that unnecessary changes are not only an issue during code review but also during subsequent maintenance. Everybody interested in the history of this file will see this commit (with a 3 lines payload) as a major change (and wonder what is the significant change?).

I don't mind genuine simplifications and refactorings that support the actual (semantic) code change, but I don't see any of this in this PR.

With that I'd prefer a bare-bones PR with only the necessary changes.

nettozahler commented 4 weeks ago

It seems I have to learn which sort of PRs are welcome for Eclipse and which are not. Obviously doing more than the absolute minimum is discouraged. Maybe I will start a discussion thread to find out more background for this restrictive approach and collect pros and cons (which exist IMO). I think I quite understand the maintainer's view but I also see some implications for contributors.

stephan-herrmann commented 4 weeks ago

It seems I have to learn which sort of PRs are welcome for Eclipse and which are not. Obviously doing more than the absolute minimum is discouraged. Maybe I will start a discussion thread to find out more background for this restrictive approach and collect pros and cons (which exist IMO). I think I quite understand the maintainer's view but I also see some implications for contributors.

I hope I didn't sound like I would speak for all of Eclipse. But it's probably safe to say that in compiler land we are more "conservative" than other parts of Eclipse. Experience tells us that a lean history is one of our most valuable assets, since many code changes start with studying the history to learn why a particular paragraph of code looks the way it does.

And as said, any changes in preparation are fine if it can be argued in which way those facilitate the 'real' change. So reducing the change to a minimum actually reduces the burden to explain your changes :)

stephan-herrmann commented 1 week ago

I don't have a windows box for testing the change. @jarthana can you help?

Meanwhile, @nettozahler you could further help us by providing a unit test. I think org.eclipse.jdt.compiler.tool.tests.CompilerToolTests would be a good place where you could take inspiration from existing tests that exercise ClasspathJsr199.

nettozahler commented 5 days ago

OK, I have taken the proposed inspiration and added a testcase.