An extension point allowing third parties to override source to dom tree implementations

[robstryker]

What it does

This extension point would allow third party products to override the conversion between java source files and the JDT dom tree with a custom implementation. In order to prevent rogue extenders from simply breaking people's installations, a system property selecting the preferred ICompilationUnitResolver must be set, as well.

As consumers of JDT become more diverse and also have been operating in environments not previously expected, being able to properly define the borders and interactions between and among the various parts of JDT becomes important. As resources to maintain various parts of the codebase become scarce, allowing the community to experiment with potentially more efficient solutions also becomes important.

Ensuring that the community can contribute to a common codebase for the vast majority of the shared work is critical to providing that community the cohesion, common goals, and shared visions that maintains a project. And allowing different elements of the community the freedom, via extension points or otherwise, to investigate alternatives to parts of the current codebase will serve to keep the project growing long term.

How to test

Other than exposing an extension point, this change does not significantly change any behavior. To test, one could attempt to make a plugin using this extension point and verifying that the various interface methods are called in a runtime workbench. I am not familiar with how the jdt.core project prefers to automatically test extension points, especially ones that require a sysprop to also be set.

Author checklist

• I have thoroughly tested my changes
• The change is following the coding conventions
• I have signed the Eclipse Contributor Agreement (ECA)

[robstryker]

@testforstephen Would you mind giving a review on this PR?

Thanks.

[testforstephen]

Would you mind giving a review on this PR?

sure, I'll take a look next week.

[rgrunber]

@testforstephen , also something to consider. I noticed your compiler extension PR extends via system property containing the FQN + Class.forName(..) on it. Rob's current approach is to use the extension point framework. It seems like it would be better for all extensions to standardize on a single approach.

[testforstephen]

Overall, the extension point looks clean to me. Could you add some unit tests for it?

[testforstephen]

@testforstephen , also something to consider. I noticed your compiler extension PR extends via system property containing the FQN + Class.forName(..) on it. Rob's current approach is to use the extension point framework. It seems like it would be better for all extensions to standardize on a single approach.

I'd keep the compiler extension PR as it is for now. If we encounter any limitations with this approach, we can consider changing it to an extension point later.

[robstryker]

Working on test cases. Will update PR asap.

[jukzi]

Please make sure that only copies or immutable data structures are passed to third party to prevent that changes by third party can make errors in jdt. IProgressMonitor is an exceptional case.
int is OK,
String is OK,
Arrays are mutable -> copy or better use a immutable List.
Map -> make a immutable copy
FileASTRequestor is mutable -> i don't have solution for that at hand

[mickaelistria]

The FileASTRequestor is intentionally mutable. It's even expected that one of its field gets modified as part of the process. It kind of captures some of the state. I believe we won't be able to change that.
For other structure, I think we should go to copies over immutable. The issue with immutable is that the immutability doesn't surface in the API, so one can try to change them and get a crash at runtime, which is a worse behavior than changing something which doesn't have an effect outside of the change scope.

[robstryker]

Please make sure that only copies or immutable data structures are passed to third party to prevent that changes by third party can make errors in jdt. IProgressMonitor is an exceptional case. int is OK, String is OK, Arrays are mutable -> copy or better use a immutable List. Map -> make a immutable copy FileASTRequestor is mutable -> i don't have solution for that at hand

I've updated arrays and maps to be cloned or immutable before sending them to the unitResolver.

[mickaelistria]

Is anything else needed before this can be merged?

[testforstephen]

It has some formatting issues, others look good to me.

would be good if others review as well

[mickaelistria]

Unless I missed one, all former review comments are now covered.

[testforstephen]

LGTM

[mickaelistria]

@iloveeclipse I appended a commit that should cover all your comments.

[mickaelistria]

@iloveeclipse I think all your review comments were addressed, can you please have another look?

[iloveeclipse]

All concerns cleared.

[akurtakov]

Are we waiting for someone else to review?

[akurtakov]

Unless there is some concern I plan to merge this one on Monday.

[akurtakov]

3 committers approved the patch - merging.

[mickaelistria]

Thanks to everyone involved!