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

Simplify logic surrounding building of Java & Kotlin parsers. #665

Merged
merged 1 commit into from
Nov 28, 2023

Conversation

timtebeek
Copy link
Contributor

What's changed?

What's your motivation?

Readability suffered a bit when mentally trying to parse what we had before.

Stream<? extends SourceFile> cus = mainJavaSources.isEmpty() ? Stream.empty()
        : Stream.of(javaParserBuilder).map(JavaParser.Builder::build)
                .flatMap(parser -> parser.parse(mainJavaSources, baseDir, ctx));

Then further improvements from there on as spotted.

Have you considered any alternatives or workarounds?

Could have merely gone for javaParserBuilder.build() in ternary, but that didn't yet help readability much.

Also simplify logic related to log statements, markers & return value
@timtebeek timtebeek self-assigned this Nov 25, 2023
@timtebeek timtebeek added the enhancement New feature or request label Nov 25, 2023
//Filter out any generated source files from the returned list, as we do not want to apply the recipe to the
//generated files.
Path buildDirectory = baseDir.relativize(Paths.get(mavenProject.getBuild().getDirectory()));
Stream<SourceFile> sourceFiles = Stream.concat(parsedJava, parsedKotlin).filter(s -> !s.getSourcePath().startsWith(buildDirectory));
Stream<SourceFile> sourceFiles = Stream.concat(parsedJava, parsedKotlin)
.filter(s -> !s.getSourcePath().startsWith(buildDirectory))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it would be even better if we could filter out these generated sources before parsing them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had thought we parse them such that we have their types available for attribution, as indicated on line 302 above. What improvement would you make here then?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose we have to test this, but these sources should still not need to be parsed, because the type attribution in other sources is via class files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it boils down to the question if getCompileClasspathElements() returns the classes compiled fom generated sources as well, which could depend on a number of different factors.

logInfo(mavenProject, "Parsing source files");
List<Path> dependencies = mavenProject.getCompileClasspathElements().stream()
.distinct()
.map(Paths::get)
.collect(toList());
JavaTypeCache typeCache = new JavaTypeCache();
javaParserBuilder.classpath(dependencies).typeCache(typeCache);
kotlinParserBuilder.classpath(dependencies).typeCache(new JavaTypeCache());

Right now we define generated sources as anything we find in the target/ directory, and parse those as source rather than compiled classes.

// Some annotation processors output generated sources to the /target directory. These are added for parsing but
// should be filtered out of the final SourceFile list.
List<Path> generatedSourcePaths = listJavaSources(mavenProject.getBasedir().toPath().resolve(mavenProject.getBuild().getDirectory()));
List<Path> mainJavaSources = Stream.concat(
generatedSourcePaths.stream(),
listJavaSources(mavenProject.getBasedir().toPath().resolve(mavenProject.getBuild().getSourceDirectory())).stream()
).collect(toList());

While I agree it might be interesting to not parse those generated sources as source files but rather only as classpath entries, I'm not entirely sure that would work in all cases, and verifying that would take more time than I currently have available. Are you ok merging this PR as-is to at least simplify the logic?

@timtebeek timtebeek merged commit 9a9428f into main Nov 28, 2023
1 check passed
@timtebeek timtebeek deleted the avoid-building-parsers branch November 28, 2023 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants