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.

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

API: make api not depend on antlr by splitting DetailAST between implementation and interface #3417

Closed
romani opened this issue Aug 26, 2016 · 16 comments

Comments

4 participants
@romani
Copy link
Member

commented Aug 26, 2016

this issue is for major version bump release.

api should be clean and do not depend on any thirdparty libraries.

antlr dependency was beneficial for quick development , but now it cause a problems during embedding checkstyle in other systems. Antlr classes could conflict with another dependencies of application.
Dependencies to any other third party is also not desirable.

from discussion with eclipse-cs author:
"
Checkstyle itself gains little to nothing from restricting packages. However in OSGI context a bundle (e.g. checkstyle-core) needs declare which of it's packages are visible to extensions.
Today this includes all of Checkstyle's packages and Checkstyle's own dependenceis (antlr, guava, commons-*).

The downside of this in OSGI realm is that those packages are now visible not just to Checkstyle extensions but also do unrelated plugins, which potentially import one of the 3rd-party packages.
This can lead to nasty class version conflicts in unexpected places, e.g. the Checkstyle plugin can potentially break a different plugin.

The main problem is in fact that 3rd-party dependencies today need to be exported in order for (some) checkstyle extensions to work.
"


MIGRATION NOTES: all extensions of Checkstyle has to be recompiled with 8.21 version of checkstyle . Example of failure - #3417 (comment)

@romani romani added the approved label Aug 26, 2016

@rnveach

This comment has been minimized.

Copy link
Member

commented Nov 3, 2016

@romani I think this can have the checkstyle 8 label.

@rnveach

This comment has been minimized.

Copy link
Member

commented Jun 26, 2017

@MEZk @romani Right now issue is you don't want to include ANTLR in API packages. DetailAST includes this because of it's class hierarchy and needing some classes for it to function and such.

DetailAST is slightly required for #4419 (comment) and there is talks of breaking apart DetailAST to uniquely identify it without the class, which I don't personally like the looks of.

However, I noticed that this issue doesn't affect DetailNode which also relies on ANTLR. This is because it is a simple interface and includes no references back to ANTLR.
For this issue, should we do the same to DetailAST? Convert it to a simple interface, and move the class' implementation to another location outside of API.

@MEZk

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2017

@rnveach
I like the idea of giving the interface to the API clients instead of the implementations.

@romani

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2017

yes, It is better to use interface, in general I agree, BUT:

  • DetailNode was created as internal API for now, and we still did not make a decision to stay with arrays in interface or make it a List (DetailNode[] getChildren(); vs List<DetailNode> getChildren();). Array is old fashioned but strict in size, List is not immutable by type (could be non-modifiable by some implementation, but ....).

  • if we make such a revolution in API, I would rather think about it throughly as it will force all Checks to be affected. Alternative is to convert DetailAST to interface and keep all methods so Checks should not be affected if recompiled. In run time old binaries might fail with errors if smth was known as class now become an interface. But this need to be tested.

  • I try to avoid one more API that will keep it possible to look at AST as mutable tree.

=================

But probably we could make it into 2 steps: convert to interface and resolve API dependency to ANTRL ; migrate to new AST that is immutable.

@MEZk

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2017

Array is old fashioned but strict in size, List is not immutable by type (could be by some implementation, but ....).

We had the discussion that we could use fully immutable collections. Collections are easier to use, but

  • this will lead to addition of a new dependency
  • additional memory
  • 3rd party lib in API

Thus, I'm for arrays.

I try to avoid one more API that will keep it possible to look at AST as mutable tree.

We can make our inner implementation immutable.

migrate to new AST that is immutable.

Should this AST class be our inner immutable implementation of the DetailAST interface which we will expose to API clients?

@jbduncan

This comment has been minimized.

Copy link

commented Jul 19, 2017

I don't know if this has been considered already, but AFAICT Checkstyle depends on Guava in some way, so one potential way of achieving List<DetailNode> getChildren(); whilst maintaining runtime immutability would be to return a com.google.common.collect.ImmutableList. Since ImmutableList implements List, this would allow the return type of getChildren(); to be List<DetailNode> whilst disallowing operations like List.set() at runtime, which would otherwise be possible to do on a mutable list or array.

@romani

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2017

whilst maintaining runtime immutability would be to return a com.google.common.collect.ImmutableList

unfortunately dependency to guava is just another pain for applications/plugins that embed checkstyle.
Ones jdk have similar collections, we will drop guava usage completely. Guava is already restricted to minimum in our project. API should not depend on Guava for sure.

@jbduncan

This comment has been minimized.

Copy link

commented Jul 19, 2017

@romani Okay. IIUC, you think my idea wouldn't work not because you think my idea is to have an API like ImmutableList<DetailAST> getChildren(); (which, to clarify, is not what I had in mind), but because the Checkstyle team is trying to minimise all sorts of dependencies on Guava, including implementation detail dependencies? :)

@romani

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2017

Api signature should not depend on guava, implementation could return runtime-immutable type.
But only and only when implementation of DetailAst is moved out api package.

Right now, DetailAST is implementation in api area . So any update to use guava will result of api dependency to guava that we try to avoid

@romani

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2017

unfortunate fact, just a reminder that API already depends on guava (8 years in play already):

$ grep ".google." src/main/java/com/puppycrawl/tools/checkstyle/api/*
.../api/Configuration.java:import com.google.common.collect.ImmutableMap;
.../api/Context.java:import com.google.common.collect.ImmutableCollection;
.../api/FileContents.java:import com.google.common.collect.ImmutableMap;
.../api/FileText.java:import com.google.common.io.Closeables;

@rnveach

This comment was marked as outdated.

Copy link
Member

commented May 4, 2018

This issue is blocked by #4609 which is to make an interface for DetailAST.

rnveach added a commit to rnveach/checkstyle that referenced this issue Apr 3, 2019

rnveach added a commit to rnveach/checkstyle that referenced this issue Apr 3, 2019

rnveach added a commit to rnveach/checkstyle that referenced this issue Apr 3, 2019

rnveach added a commit to rnveach/checkstyle that referenced this issue Apr 5, 2019

rnveach added a commit to rnveach/checkstyle that referenced this issue Apr 6, 2019

rnveach added a commit to rnveach/checkstyle that referenced this issue May 4, 2019

rnveach added a commit to rnveach/checkstyle that referenced this issue May 4, 2019

@rnveach rnveach changed the title api changes: make api do not depend on antlr API: make api not depend on antlr by splitting DetailAST between implementation and interface May 7, 2019

@rnveach rnveach added this to the 8.21 milestone May 7, 2019

rnveach added a commit that referenced this issue May 7, 2019

@rnveach

This comment has been minimized.

Copy link
Member

commented May 7, 2019

Fix was merged.

@rnveach rnveach closed this May 7, 2019

@rnveach

This comment has been minimized.

Copy link
Member

commented May 7, 2019

@romani I assume you are ok with misc label?

@romani

This comment has been minimized.

Copy link
Member Author

commented May 19, 2019

I put breaking compatibility, just be make it visible, as we do not know how much our API is used.

@romani

This comment has been minimized.

Copy link
Member Author

commented May 26, 2019

@rnveach ,

We did very big compatibility damage - https://travis-ci.org/sevntu-checkstyle/checkstyle-samples/jobs/537427424#L522 , please recheck why it was not detected during testing.

What we can do to prevent this in future ? execute samples in checkstyle's CI over snapshot ?

[checkstyle] Running Checkstyle 8.21 on 1 files
BUILD FAILED
/home/travis/build/sevntu-checkstyle/checkstyle-samples/ant-project/build.xml:15: 
java.lang.Error: Error was thrown while processing 
/home/travis/build/sevntu-checkstyle/checkstyle-samples/ant-project/src/com/github/sevntu/checkstyle/Sample.java
	at com.puppycrawl.tools.checkstyle.Checker.processFiles(Checker.java:315)
	at com.puppycrawl.tools.checkstyle.Checker.process(Checker.java:220)
	at com.puppycrawl.tools.checkstyle.ant.CheckstyleAntTask.processFiles(CheckstyleAntTask.java:349)
	at com.puppycrawl.tools.checkstyle.ant.CheckstyleAntTask.realExecute(CheckstyleAntTask.java:316)
	at com.puppycrawl.tools.checkstyle.ant.CheckstyleAntTask.execute(CheckstyleAntTask.java:288)
	at org.apache.tools.ant.UnknownElement.execute(UnknownElement.java:292)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.apache.tools.ant.dispatch.DispatchUtils.execute(DispatchUtils.java:106)
	at org.apache.tools.ant.Task.perform(Task.java:348)
	at org.apache.tools.ant.Target.execute(Target.java:435)
	at org.apache.tools.ant.Target.performTasks(Target.java:456)
	at org.apache.tools.ant.Project.executeSortedTargets(Project.java:1393)
	at org.apache.tools.ant.Project.executeTarget(Project.java:1364)
	at org.apache.tools.ant.helper.DefaultExecutor.executeTargets(DefaultExecutor.java:41)
	at org.apache.tools.ant.Project.executeTargets(Project.java:1248)
	at org.apache.tools.ant.Main.runBuild(Main.java:851)
	at org.apache.tools.ant.Main.startAnt(Main.java:235)
	at org.apache.tools.ant.launch.Launcher.run(Launcher.java:280)
	at org.apache.tools.ant.launch.Launcher.main(Launcher.java:109)
Caused by: java.lang.IncompatibleClassChangeError: 
Found interface com.puppycrawl.tools.checkstyle.api.DetailAST, but class was expected
	at com.github.sevntu.checkstyle.checks.coding.ReturnNullInsteadOfBooleanCheck.visitToken(ReturnNullInsteadOfBooleanCheck.java:78)
	at com.puppycrawl.tools.checkstyle.TreeWalker.notifyVisit(TreeWalker.java:363)
	at com.puppycrawl.tools.checkstyle.TreeWalker.processIter(TreeWalker.java:470)
	at com.puppycrawl.tools.checkstyle.TreeWalker.walk(TreeWalker.java:304)
	at com.puppycrawl.tools.checkstyle.TreeWalker.processFiltered(TreeWalker.java:162)
	at com.puppycrawl.tools.checkstyle.api.AbstractFileSetCheck.process(AbstractFileSetCheck.java:85)
	at com.puppycrawl.tools.checkstyle.Checker.processFile(Checker.java:332)
	at com.puppycrawl.tools.checkstyle.Checker.processFiles(Checker.java:294)
	... 21 more
@rnveach

This comment has been minimized.

Copy link
Member

commented May 26, 2019

@romani I can only assume it is because when we test with sevntu we re-compile the classes and that is why we missed it. For our own sevntu run in the pom, we still use an old version of checkstyle since sevntu is still connected to it.
To catch this in the future, checkstyle snapshot would have to run with the released version of sevntu and not compile it from scratch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.