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

fix two problems in special kind handling #23

Closed

Conversation

KengoTODA
Copy link
Member

This MR is same with findbugsproject/findbugs#86

1. defineNewSpecialKind(String) method returns an int value which is not same with actual newly defined SpecialKind
2. specialKindNames field is not thread-safe, even though it is annotated with @StaticConstant so it will be shared between analysis runs.

refs findbugsproject/findbugs#86

Squashed commit of the following:

commit bb10ef2bd8ee4816f7b6bc0c4f4e855b9de5b144
Author: Kengo TODA <skypencil@gmail.com>
Date:   Fri Feb 19 05:29:35 2016 +0800

    improve javadoc

commit 3ead4633ec81aafbbe744f16d9bdaaf572b059ef
Author: Kengo TODA <skypencil@gmail.com>
Date:   Fri Feb 19 05:20:21 2016 +0800

    bugfix: StaticConstant should be threadsafe

commit 5fae95bd38d56a0620edf094af3cca864b10023b
Author: Kengo TODA <skypencil@gmail.com>
Date:   Fri Feb 19 05:16:28 2016 +0800

    change behaviour of #defineNewSpecialKind(String) to make it intuitive

commit 8e8de1fc958beca01205d93acb94e5d1a3ae7aa6
Author: Kengo TODA <skypencil@gmail.com>
Date:   Fri Feb 19 05:12:52 2016 +0800

    reproduce strange behavior
@jsotuyod
Copy link
Member

This looks great. I don't see much of an issue with changing types for specialKindNames. Since ConcurrentMap extends Map we are safe for well-behaved code.

The only breaking cases would be:

  • Storing it to a local HashMap
  • Passing it to a method that expects a HashMap

Good practice calls for people to access it by interface as a Map (which SpotBugs should have done either way), and even if not, it's a trivial error to spot (it will throw a compile error or runtime exception) and fix so I'm not particularly against breaking this API.

On the contrary, I'm hesitant on changing defineNewSpecialKind. Even though it's true it's return value is off-by-one, it's been broken for almost 4 years.

This could either mean, no one is using it and we are safe, or they just workaround it on their own, meaning this fix would break that code. If it's the second scenario, the fix may go unnoticed and cause more harm than good (plugins may not break, though they would misbehave).

I've so far failed to find a plugin that uses defineNewSpecialKind so I tend to think we are mostly on the first scenario, but I wonder what @iloveeclipse and @mebigfatguy think, since both of them were around at the time of the breaking change.

@jsotuyod
Copy link
Member

Unless @mebigfatguy has some insight, I guess the best we can do without breaking third parties is:

  1. @Deprecate the defineNewSpecialKind method, leaving it's implementation as-is.
  2. create a new method named defineSpecialKind or whatever you see fit, with the correct behavior.

People using the old code will work as expected, new users can use the good method, and in a future SpotBugs 4 release, we can safely kill the deprecated method.

Comments anyone?

@mebigfatguy
Copy link
Contributor

@jsotuyod 's approach seems sane. i really have no clue about that code

@KengoTODA
Copy link
Member Author

@mebigfatguy @jsotuyod Thanks for your review! I made another pull-request #27 to add methods, please check. I will close this pull-request.

@KengoTODA KengoTODA closed this Nov 15, 2016
@KengoTODA KengoTODA deleted the fix-define-new-special-kind branch November 15, 2016 00:55
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 this pull request may close these issues.

None yet

4 participants