eclipse-jdt / eclipse.jdt.core

Eclipse Public License 2.0
157 stars 125 forks source link

Computation of Exclude/Include patterns in a non-persitent way by an extension #642

Open laeubi opened 1 year ago

laeubi commented 1 year ago

Currently I can define exclude/include items from classpath by specifying a pattern:

grafik

this is then stored in the .classpath like this:

<classpathentry excluding="**/example/*.java" kind="src" output="target/test-classes" path="src/test/java">

What I like to archive is, that this value is not persistently stored in the classpath, but computed by an extension point, such an extension point should get the IClasspath element (and if possible the I(Java)Project) and would return then include/exclude patterns that should apply to this classpath element.

@stephan-herrmann what do you think? This would be a more generic replacement of the enable/disable, and actually we already use this in m2e, but only compute it once and store it in the .classpath, this would also help with:

PDE can simply then implement this extension and filter out all classes that do not match the current target environment.

One problem I see is that when to actually this must be applied, e.g. when resolving classpath? but this is also shown in the UI as "disabled", so the UI must probably also call the extension point.

iloveeclipse commented 1 year ago

Isn't there already IClasspathContainer for that?

laeubi commented 1 year ago

Isn't there already IClasspathContainer for that?

IClasspathcontainer hosts the classpath entries yes, but I'm not sure how it helps here?

stephan-herrmann commented 1 year ago

Isn't there already IClasspathContainer for that?

IClasspathcontainer hosts the classpath entries yes, but I'm not sure how it helps here?

The mechanism of classpath container and classpath container initializer translates ("resolves") one container entry into a set of concrete entries, typically jars or other projects. The cool thing here is, that the extension implementing container resolution is free to create the concrete classpath entries with any details they wish, e.g, the PDEClasspathContainer interprets MANIFEST.MF not only for identifying all required bundles, but also adds access rules to each entry, reflecting the details of the target manifest (x-internal and x-friends).

For a recent example you may have a look at Bug 526011 (gerrit) where I use a new manifest entry to generate an extra classpath atttribute "annotationPath".

In short, any information available to the extension (implementation of a classpath container) and which can be represented already in terms of classpath attributes, can be dynamically passed into JDT with this mechanism. Plus: JDT even remembers attributes on resolved classpath entries in its persistent State, where resolution of classpath containers is cached for improved start-up time.

Thanks, @iloveeclipse for reminding us of this powerful concept :)

laeubi commented 1 year ago

Well but I don't want to implement my own ClasspathContainer I want to modify the behavior of JDT ones (see screenshot above) or probably all (if include/exclude applies to every classpath container).

stephan-herrmann commented 1 year ago

(see screenshot above)

The entry is partly hidden, but you seem to be talking about a source entry, not a container entry, right?

I wonder if it would be possible to wrap even source folders inside a new filtering classpath container. Let me see ...

laeubi commented 1 year ago

(see screenshot above)

The entry is partly hidden, but you seem to be talking about a source entry, not a container entry, right?

I wonder if it would be possible to wrap even source folders inside a new filtering classpath container. Let me see ...

IClasspathEntry can be of type

and has the getExcludePattern() / getIncludePattern()

IClasspathContainer.getClasspathEntries() as per javadoc only contains CPE_LIBRARY or CPE_PROJECT so I'm not sure if that helps me here.

stephan-herrmann commented 1 year ago

IClasspathContainer.getClasspathEntries() as per javadoc only contains CPE_LIBRARY or CPE_PROJECT so I'm not sure if that helps me here.

I could not find any code enforcing this. So perhaps this is just how it was used up until now?

stephan-herrmann commented 1 year ago

I could not find any code enforcing this. So perhaps this is just how it was used up until now?

Correction: org.eclipse.jdt.internal.core.ClasspathEntry.validateClasspathEntry(IJavaProject, IClasspathEntry, IClasspathContainer, boolean, boolean) would need to be modified. But I think this could be possible without negative impact.

stephan-herrmann commented 1 year ago

Looking at this validation

    if (containerEntry == null
        || kind == IClasspathEntry.CPE_SOURCE
        || kind == IClasspathEntry.CPE_VARIABLE
        || kind == IClasspathEntry.CPE_CONTAINER){
            return new JavaModelStatus(IJavaModelStatusConstants.INVALID_CP_CONTAINER_ENTRY, project, path);
    }

the purpose of disallowing CPE_VARIABLE and CPE_CONTAINER seems to be to avoid the need of nested resolving (i.e., entries of a resolved container should be directly usable).

Excluding also CPE_SOURCE might just have been a conservative approach, since no use case existed for this?

Should we try to remove just that line and play with a container wrapping source folders?

stephan-herrmann commented 1 year ago

I found one location assuming that CPE_SOURCE can be identified using the raw (unresolved) classpath: org.eclipse.jdt.internal.core.JavaProject.isOnClasspath(IJavaElement), the following snippet would need an update, too:

        // no need to go further for compilation units and elements inside a compilation unit
        // it can only be in a source folder, thus on the raw classpath
        if (isSource)
            return false;

but that looks feasible, if you ask me.

laeubi commented 1 year ago

I'm not sure, as I said I don't want a custom container, I just want to specific (dynamically) include/excludes for the existing JDT (source?) classpath entry :-)

stephan-herrmann commented 1 year ago

I'm not sure, as I said I don't want a custom container, I just want to specific (dynamically) include/excludes for the existing JDT (source?) classpath entry :-)

You make it sound like inventing a new extension point is easier than using (and marginally expanding) an existing one. I believe the opposite is the case. :)

laeubi commented 1 year ago

Bu what extension point do you like to expand? Sources are not Classpathcontainers, and even if they would it won't help me as I don't want to add one :-)

Look for example at org.eclipse.jdt.internal.corext.buildpath.ClasspathModifier.isExcluded(IResource, IJavaProject) ther it checks if an item is excluded (and there is isIncluded as well) and thats effectivly where I need to intercept, so:

Lets say isExcluded currently return true at the momment, I'd like my extension to be asked so I can change the answer to false ...

stephan-herrmann commented 1 year ago

Bu what extension point do you like to expand?

It's not about like or not, but about assessing different possible solutions for a given requirement. For the concrete purpose here I suggested to consider lifting just one restriction of IClasspathContainer.getClasspathEntries() such that it admits CPE_SOURCE, too. That would increase the flexibility of the existing API to cover your use case with no new API, at least no new extension point.

it won't help me as I don't want to add one :-)

Well, if you already know which solution you don't want, then finding the most appropriate solution may be difficult.

My reasoning is based on the idea that adding / modifying API / extension points is much, much more harder to get right, than adding implementation against existing API. And if we release API that is not good, then our options to improve are not great.

As we have been discussing related topics in different issues now, I would suggest, to list all current and known future requirements in one place, and collectively try to find the smallest API surface to cover them all. Will you join me in this?

laeubi commented 1 year ago

For the concrete purpose here I suggested to consider lifting just one restriction of IClasspathContainer.getClasspathEntries() such that it admits CPE_SOURCE, too.

But just assume IClasspathContainer admits CPE_SOURCE how will it help with the case having additional filtering for what is currently there? This is not related about the container at all... the container can stay as is.

As we have been discussing related topics in different issues now, I would suggest, to list all current and known future requirements in one place, and collectively try to find the smallest API surface to cover them all. Will you join me in this?

Just let me know where to join in ;-)

iloveeclipse commented 1 year ago

The idea is that similar to PDE's "org.eclipse.pde.core.requiredPlugins" the maven provider dynamically computes everything, but now also where sources are. So maven / m2e does all the classpath work instead of JDT specifying that in the .classpath.

laeubi commented 1 year ago

The idea is that similar to PDE's "org.eclipse.pde.core.requiredPlugins" the maven provider dynamically computes everything, but now also where sources are.

m2e already has a classpath container (named "Maven Dependencies"), so you mean one should remove the items in .classpath and instead return CPE_SOURCE entries (together with the jar items)? If yes I think that could be interesting, and if you think JDT can handle this ... even PDE has "special" files (build.properties) already that duplicates some information from the classpath.

I'm just was curious because classpath kind="src" looks so general and special so I can't really tell if that could cause trouble somewhere but trust you and @stephan-herrmann as the JDT experts here :+1: