eclipse-jdt / eclipse-jdt/eclipse.jdt.core
Batch compiler is inconsistent about separators for package names
- Dominant language
- Java
- Stars
- 237
- Forks
- 195
- Avg merge
- 1d 12h
- Merged PRs (30d)
- 47
Description
Continuation from my [previous analysis](https://github.com/eclipse-jdt/eclipse.jdt.core/issues/1452#issuecomment-1741811631): code in package `org.eclipse.jdt.internal.compiler.batch` doesn't have any obvious system which separator (`.`, `/`, `File.separatorChar`) should be used where.
Two particular problems to be called out:
1. Code in `FileSystem` indicates that different implementations of methods in `FileSystem.Classpath` might have different contracts:
* it affects `findClass(char[], String, String, String, boolean)` and `findTypeNames(String, String)`
* two methods in `FileSystem` perform `instanceof` checks to figure out which separators should be used before using one of these methods.
* sounds like the principle of maximum surprise :)
2. `o.e.j.c.dom.NameEnvironmentWithProgress.findType(char[], char[][], boolean, char[])` indirectly invokes `ClasspathDirectory.getSecondaryTypes(String)` with a `/` separated name, where apparently the callee expects a `File.separatorChar` separated name.
* minimally this could cause unnecessary cache miss vis-a-vis `ClasspathDirectory#packageSecondaryTypes` and `#directoryCache`
I didn't find any direct confusion regarding `.` separated names, but from looking at method signatures and variable names this isn't fully obvious either.
Ideally, I would like to remove all references to `File.separatorChar`, using `/` throughout, given that even Windows should be able to correctly interpret such paths.
If that's too optimistic, a general overhaul of this code section might be called for:
* documentation of which separator should be used where
* perhaps a naming convention to support explaining any contracts (e.g., `packageName`, vs. `packagePath` vs `platformPackagePath`.)
* removal of unsystematic conversions and conditional code
Contributor guide
Assessment
This issue has not been assessed yet.