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

CollectionIncompatibleType bug in TerminalBuilder found by Error Prone #846

Closed
vorburger opened this issue Jun 19, 2023 · 5 comments
Closed
Milestone

Comments

@vorburger
Copy link
Contributor

I'm running https://errorprone.info on this project, and it claims to have found a bug (line numbers are as of 3.23.0):

terminal/src/main/java/org/jline/terminal/TerminalBuilder.java:407: error: [CollectionIncompatibleType] Argument 'l' should not be passed to this method; its type TerminalProvider is not compatible with its collection's type argument String
            int idx = order.indexOf(l);

See https://errorprone.info/bugpattern/CollectionIncompatibleType ... @gnodet from a quick glance, I think this is a real bug, not a false positive, this does seem wrong (because it mixes TerminalProvider and String):

List<TerminalProvider> providers;
List<String> order;

providers.sort(Comparator.comparing(l -> {
            int idx = order.indexOf(l);
            return idx >= 0 ? idx : Integer.MAX_VALUE;
        }));

@cushon just FYI

@vorburger
Copy link
Contributor Author

FYI I've not actually really directly run ErrorProne on JLine, but I'm building JLine in an internal build system at work where this pops up. I have created #847 re. actually adding ErrorProne to jline3 (as in, to the pom.xml of this project / Git repo), so this issue can be about the specific issue above, only.

@jvalkeal
Copy link
Contributor

Yes, that is a bug. Even eclipse/vscode gives warning for it

Unlikely argument type TerminalProvider for indexOf(Object) on a List

Should be

providers.sort(Comparator.comparing(l -> {
	int idx = order.indexOf(l.name());
	return idx >= 0 ? idx : Integer.MAX_VALUE;
}));

and sorting starts to work.

@gnodet
Copy link
Member

gnodet commented Jul 27, 2023

Nice, can one of you create a PR for that ?

@jvalkeal
Copy link
Contributor

Yes, I can push a PR

jvalkeal added a commit to jvalkeal/jline3 that referenced this issue Jul 28, 2023
- Change provider sorting so that it uses provider name instead
  of provider object itself. This fixes getting a proper sorting
  list index to compare the order. Uses null check in case provider
  wrongly gives name as null.
- Fixes jline#846
@jvalkeal
Copy link
Contributor

@gnodet Did a pr #852 for this.

@gnodet gnodet added this to the 3.23.1 milestone Jul 31, 2023
gnodet pushed a commit to gnodet/jline3 that referenced this issue Jul 31, 2023
- Change provider sorting so that it uses provider name instead
  of provider object itself. This fixes getting a proper sorting
  list index to compare the order. Uses null check in case provider
  wrongly gives name as null.
- Fixes jline#846, jline#852
@gnodet gnodet closed this as completed in 3c93ea7 Aug 1, 2023
@gnodet gnodet modified the milestones: 3.23.1, 3.24.0 Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants