Do not emit restriction warning on inherited method#4293
Conversation
|
Short answer: I'm not convinced. If you look in package p1, there is no such method, right? If p2 is an internal package, then the owner of that package is allowed to do things like removing For such reasons, exporting a class that inherits from an internal class is inherently fragile / breaks encapsulation. And if p2 is not on the clients classpath they will get "The type B cannot be resolved. It is indirectly referenced ..." |
Outline tells me something different, also javadoc, code complete,... all tell me
Given this class is extended by legal consumers this will likely break them as the probably calling or overriding the method, so I don't think it is allowed. Also I don't think a restricted package is necessarily internal. If any public code is referencing this in its API it at least (e.g. extend an abstract class with public methods) leaking that (internal) API to the outside.
That might be the case, but I don't think that restrictions really make such strong assumption, it just tells you should not directly reference the class. So now we assume your case of someone really really really wants to remove method
that's why only a direct reference to |
None of them speak of showing the contents of p1. They know about that method because they look into p2, despite the restriction. Anyway, I have more urgent tasks waiting for me than bending the rules of classpath restrictions. |
But that's why we want it on the classpath and instead using access rules right?
I must confess I never have heard about a method belong to a package and I would have not even bothered to ask or change anything if not even the compiler seem to emit a call to And while it makes perfectly sense for a deprecated method to be checked "at the source" I don't get the rationale for an access restriction and even the code seem to indicate with a So I went to the code and checked the callers of the method and found this place, as the exact same is used for implicit constructor calls in Then I looked at the compiler documentation here and it tells me
so it always talks about types not methods of a package. Also please note that there is a type checking also in place with
I really don't want to bend any rules or waste your time, in fact I really appreciate your insights and got the feeling in the past we found some good solutions that are beneficial for the IDE story in general even though the path not always was a direct one. But then I don't understand your recommendation here, as this example shows why PDE currently can not effectively use access restriction rules to mitigate the problem. In OSGi one always "opens" packages not types or methods, that mean if I export Type So I really would appreciate any guidance here, because this is one of the problems I really like getting solved as it is highly confusing, requires manual fixing by the user and makes the tooling look brittle compared to other solutions. If it helps I would be fine with even introduce a new restriction type, but as explained above it does not feel right and seems a bit of an overkill here. |
|
I see one source of confusion in the fact that packages are used as modules, whereas they don't have a clear specification of what are its dependencies and its API - this sounds like an underspecified concept. My intuition about the API of a package is: all public stuff that you can find in .java files in the package's directory. This gives the following nice guarantee: if a change in any of these files passes compilation of the current artifact, then the change is legal. Your intuition takes inheritance into consideration, which may have its own advantages. In case of differing intuitions I prefer to work from specifications. So were can we look for precise answers? (1) If your example were embedded in a JPMS module which only exports one of its packages, then - much to my surprise - clients of that module are allowed to invoke a method that is declared in a non-exported package. (I haven't checked where in JLS this is defined, but ecj and javac do agree here). Point in favor of considering inheritance. (2) Next question: Which tool will detect illegal changes in a non-exported package? Assuming that the method in question is not used within the artifact, compilation will not see any problem. Will API tools complain about changes in internal packages, if there is a class in a public package inheriting from the internal one? Ergo: (1) sets a precedent for selecting your intuition over mine, but (2) needs to be answered regarding API tools, to determine if a change in the ecj implementation can cause bad surprises. The change would allow clients to rely on methods that are declared in a restricted package. If that is so, then who / what would protect that client from incompatible changes? Will API tools put that method under the rules of exported API? |
|
BTW, did you check the history of those tests in AccessRestrictionsTests that are failing with your change? Perhaps that history would reveal the reasoning behind the current implementation? |
Yes I need to further investigate and see what cases are not covered correctly by my naive change here. Also there is a difference between forbidden references and discouraged reference and I need to check what applies here. |
I wanted to verify this as well but as I'm not a JPMS expert I was not fully able to came up with an example, so thanks for doing this experiment. I think this is because of the same reason as it "works" for OSGi, as it is only classloader visibilty. So when we have restricted Type So let it be good or bad, from a restriction point of view I never access anything that is restricted that is not restricted by reflection as well (I need access to A.class to get the restricted (non public) method handles). So I would argue, everything that can be legally accessed through reflection without requiring the specific type should be allowed when it comes to restrictions (but JDT itself is sometimes confused as seen here and here).
One thing about the usage of restricted package is, that it not necessarily mean they are not exported, but currently not imported so in some cases I can gain access to these packages of course. So currently "the tool" is the compiler (what leads to a problem if one wants to use
Now API tools complicates things a lot (and where we can not fully rely on the compiler anymore):
So API tools will need to detect a lot of more case here and some are not discoverable from the raw manifest data. You can find a list of message one would expect here: and it complains about as much as is externally visible (except it is marked as |
I can imagine a lot of complexity there, but I wouldn't require all of that to be sorted out, before continuing here. 😄 All I'm asking is this: when ecj will (per this issue) legalize access to a method that is declared in a restricted package, this will introduce a maintenance risk, unless API tools will locally detect changes that may break unsuspecting clients. Does API tools detect changes in restricted packages (not exported, or exported x-internal, or such), that may affect clients? For this we don't even need to consider restrictions due to a missing import in the client, I'm only asking about restrictions that are visible at the provider side. |
|
Its quite hard to find a suitable example but:
so we can say for sure I got warned that I do something nasty, so whatever goes wrong after that fall into the category "you got warned!"
So I think because API tools forbids that public types extend non public ones, the described problem (I inherit a public method from a non API type and later want to remove that method) should not arise as it is illegal at the first place. For the case that I extend a public type (and simply do not import the package) there is also no problem as API tools will complain because the base class is part of a public API. |
|
@stephan-herrmann I have now adjusted the first failing
if we can agree on that being the right thing, I'll look into the other failures (but they look similar). |
We had two issues in the recent past of which you probably mean the first:
|
|
@HeikoKlare thanks for the reference, did you open already corresponding issues at PDE? |
|
@stephan-herrmann any chance to get an agreement here? I think it would be good to be merged as early as possible in the release cycle. |
abcd475 to
4301208
Compare
|
@stephan-herrmann I now rebased and adjusted all testcases here, so from my side it would be ready to be submitted for inclusion in M1. |
fafcdb3 to
3d38e39
Compare
|
@stephan-herrmann build is finally green, so I think this would be ready to go if we can conclude on this. As this would really improve user experience in the tooling it would be great to make progress here and as both OSGi and JPMS agree here it sounds the right thing to do for me. |
|
@stephan-herrmann can we continue here for maybe get this into M2? Would be highly appreciated! |
|
I was about to be convinced that the change shouldn't hurt, but just upon reviewing test changes this took me back all the way to https://bugs.eclipse.org/bugs/show_bug.cgi?id=76266, which has this description:
So finally we know that the "undesired" behavior was implemented intentionally exactly that way. And Philipe is an engineer whose judgement I like to follow until proven wrong. From #4293 (comment) :
This also gives a hint that we are trying to address a problem which in a perfect world should never arise. So let's step back one more time:
|
|
From #4293 (comment)
At least this will not cause and "restriction" warnings from ecj, as |
This doesn't actually address the issue I raised. I was asking whether the event of modifying any "semi-API" method (lives in a restricted package but is exposed to the world through a subclass) would cause any tool to shout and tell the developer, that this change may break clients? The fact that the tool has already shouted before, doesn't help the client who will be broken, if no specific error is raised at the event of the method change. |
I do not want to prove anyone wrong, an maybe it would be worth to add Philipe then here to join the discussions but this was 20 years ago so maybe things have changed in the meanwhile, but what I really miss here is the actual use-case to solve? So apart from theoretical cases we might want to solve with Access restrictions, what is the practical example where it becomes useful?
So who we really benefit here? Who will use this feature if PDE + Tycho will need to drop it because it is practically unusable?
It is not surprising (despite the fact its an error), it is wrong and unintended from the users point of view, there is nothing that ever judge the warning nor is the solution (adding unrelated packages) in any way helpful or required.
Yes it does, the whole purpose of why we use this feature is to prevent developers to use types (!) that can not be accessed at runtime and will fail. Due to the nature of the problem, that it occurs on top of the class file it is completely disturbing and hard to give good guidance for the tooling so usually people have to intervene manually and are annoyed that we require them to do things that are completely useless. Even worse, a perfectly working code can suddenly break e.g. because an overridden method is removed or we clean up unused packages: It works in some cases and in other gives strange error then, depending on what methods the user calls.
That's completely unreasonable (beside it bloats the code and the classfile and maybe have runtime performance implications)
As said nothing blows up at runtime (beside that it makes it harder for the resolver to resolve packages that are never used), we just use usual java classloader visibility.
That's a completely different story and restriction warnings do not solve this because:
The point is we have tools to restrict "semi API" on methods and this is completely vendor (PDE) specific thing that has no meaning to OSGi or the runtime, so nothing will break if you use such restricted method or fields. |
|
Wow, the disconnect runs deeper than I thought. In an attempt to debug the discussion I will answer in small increments. Easiest first:
If we are speaking of the same thing, then it's each project's choice to configure this diagnostic to Error/Warning/Info/Ignore, in two separate options for "forbidden" and "discouraged". Since it's not a mandatory error (as those that are defined in JLS) I suggest to use the term warning in a generic way, knowing that warnings can be promoted or demoted in severity by the user. While I'm in favor of zero-warnings policies, there are means for coping with a small number of exceptions, like But as the PR headline speaks of "warning", this might be a simple misunderstanding, perhaps when you wrote "error" you were referring to something else? |
|
Next we should perhaps clarify which situation we are discussing: A client invokes a method that is declared in a class B, superclass of API class A. Possible reasons why this could be a problem:
I thought we are speaking of (1) but when you speak of adding unnecessary imports, this sounds more like (2). Again, I might be misunderstanding why you speak of adding more package imports. |
And here it starts to getting wrong already, you assume that is a deliberate choice of the user but its not. So here a tool is needed (PDE) to translate between OSGi world and JDT world, and it is not a question of choice but correctness, if I do not import a package of a type than it is an error without doubt. But calling a method on a public and imported type is neither an error nor a warning. So let it be a warning or an error is not that important to me, usually it leads to a compile error. |
To explain more: I had been looking for ways how tools enforce more explicit contracts, in the hope that those contracts help detecting misconfigurations. In that I failed to see, that a component may require X version 1.0, but still be compiled against X version 1.1 with no complaints, thus illegally using version 1.1 features. Whether or not that should be allowed opens another can of worms, which should not be opened here (is that the pde-bot thing?). This implies that problems caused by uncoordinated software evolution cannot easily be detected just looking at one set of components, but are better detected by version comparison. With this I'm willing to go back to MR pragmatics. @laeubi can you provide a message to adopters that will inform them about this design change in the release notes? |
But exactly that is happening here, the situation you created is a bit like Compiling with Java 11 JDK with a source compliance of 8 and don't using the target option... The compiler can simply not know that at runtime some calls will fail. One might argue the compiler can still warn you if it would do an further analysis assuming an implicit target option, but this would be likely a different tool (and there are tools for this!)
The problem is it can't if the meta-data is wrong! Assume I compile a class as compliance 11, then would edit the class file and replace the version with the one for 8 with an hex editor... no good thing could come from this. For the same OSGi (runtime) must assume the metadata in manifest is correct what is the whole purpose of it, so it does not need to analyze the classfiles for example you need to state in the Manifest what is the minimal java version you want to run with. Now it comes to the point that PDE (compile time) historically has leaned to the so-called "Manifest-first" that is a developer usually writes the manifest (I'm working on help the developer to get some errors/warning here eclipse-pde/eclipse.pde#1926 if they likely do things wrong), so we need more tools (e.g API tools) that need more data (e.g the baseline) to provide further assistance / checks at "tool time" as we all tend to make mistakes. |
|
@stephan-herrmann I proposed a N&N entry here also squashed, rebased and added a reference to this discussion in the commit message. |
Currently when we have a type A and it is accessible from package p1, but extends a type B (that is not accessible in package p2) calling any inherited public method A.M triggers a restriction warning. This is undesired because as a caller I never access any restricted code here as A is inherit all public methods from B. See eclipse-jdt#4293 for a discussion about that topic.
4c239dd to
7153d9b
Compare
Two suggestions:
thanks. |
stephan-herrmann
left a comment
There was a problem hiding this comment.
In the end the discussion produced some relevant observations which lessen the severity of my worries of changing a long standing design.
Let's go.
That wasn't so hard. Result: it doesn't detect the need to increment any version due to method added to a super class. Neither when inheriting from a class of another bundle, nor when inheritance is from a different package of the same bundle. => one more tool to be updated in order to consistently support inherited methods as API of the subclass. |
|
Does it detect other problems? It is not that easy to setup often to give good results, that's why I wrote the tycho-baseline check :-) Also API tools is often not that good at detect package-version increments, I recently added some support but there is maybe still things to fix. In this case it could be that API tools is happy because you increased the bundle version already :-\ |
Do you like to suggest a change in that PR? I can then just apply it to the entry, I'm open for any changes. |
It reported quite a number of other bugs I injected.
I played with several variants of versioning, none brought the desired warning relating to package q. |
you can do that, it's your feature :) |
I'm quite bad ad writing N&N entries but will try to improve that.. |
…jdt#4293)" This reverts commit 545483d as it caused regression in eclipse-jdt#4553 Fixes eclipse-jdt#4553
Due to eclipse-jdt/eclipse.jdt.core#4293 ecj no longer warnigs about using a method inherited from a deprecated type
Due to eclipse-jdt/eclipse.jdt.core#4293 ecj no longer warnigs about using a method inherited from a deprecated type
* Don't listen to thread termination, listen to target termination For whatever reason original change added listener on thread termination and not on target termination event, and for whatever reason it wasn't noticed during review. Fixing that. HCR dialog should only close automatically if the debug target is terminated, not if some thread in the target is terminated. Fixes #792 * Extended support for in-line chained lambdas Allow users to put a lambda entry breakpoint on a selected lambda if line contains multiple chained lambda expressions. Fixes : #732 Co-authored-by: Andrey Loskutov <loskutov@gmx.de> * Enable "Collapse" stack frames by default Fixes #796 * Remove @SuppressWarnings annotation no longer raised by ecj Due to eclipse-jdt/eclipse.jdt.core#4293 ecj no longer warnigs about using a method inherited from a deprecated type * Remove @SuppressWarnings annotation no longer raised by ecj, take 2 Fixes remaining `@SuppressWarnings("deprecation")` annotations flagged as unnecessary by eclipse-jdt/eclipse.jdt.core#4564 See eclipse-jdt/eclipse.jdt.core#4553 * React to new info warnings from ecj (#802) Add @deprecated to members of truly deprecated classes + most classes have a replacement for a long time already + still in use and no replacement specified: - JavaSourceLocationWorkbenchAdapterFactory * Disable breakpoint Entry&Exit toggle actions for lambda breakpoints (#803) Enabling exit will make debugger suspend at random lambdas and by default lambda method breakpoints are already enabled --------- Co-authored-by: Andrey Loskutov <loskutov@gmx.de> Co-authored-by: Stephan Herrmann <stephan.herrmann@berlin.de>
Currently when we have a type
Aand it is accessible from packagep1, but extends a typeB(that is not accessible in packagep2) calling any inherited methodA.Mtriggers a restriction warning.This is undesired because as a caller I never access any restricted code here as
Ais inherit all public methods fromB.A good example can be found here:
in this example the code in
HttpServerManagerhas a local variable of typeServerand calls methodstop()on this.this causes the following errors:
it does not emit such warnings if the method itself is overridden (an example would be
server.dump(null, "")that overrides a method inContainerLifeCyclefrom the same package) because then the method binding equals the actual binding.To mitigate this, the class
MessageSendnow checks if there is an implicit call and setsisExplicitUse=falselike it is already done for implicit super constructor calls.@stephan-herrmann What do you think? Any flaws in my analysis?