[External] : Re: eclipse warnings

[John Hendrikx]

I think the danger of regressions from fixing such warnings is 
incredibly low.  Fixing raw types finds potential casting problems, ie:

       List list = List.of("Hi");

       Integer x = (Integer) list.get(0);  // Runtime CCE

With the type for List specified, it would be a compile error instead.

It was deemed so important to the Java platform that it introduced 
generics in 1.5, but kept raw types as a backwards compatiblity 
mechanism (not as a "convenience" mechanism to avoid having to specify 
the generic parameters -- all code after 1.5 should specify generics, 
and never use raw types).

The PR uncovered a few incorrectly specified generics, although it was 
unlikely there were any resulting bugs, as the incorrect generics were 
in the CSS subsystem which ignores them all everywhere anyway.

The generics in the CSS subsystem were used without thinking if they 
were useful or needed, with ParsedValue being the main offender.  It 
offers zero benefits to have this one generic as you're always dealing 
with parsed input that won't have a compile time type known in advance 
(all type checking is done in the parser, manually).  Unfortunately, the 
class is public API, but causes raw type warnings throughout the whole 
CSS subsystem.  If there had been an attempt to fill in reasonable 
generic types for this class everywhere, one would have realized the 
generic parameters on this type was not very useful, and could have been 
dropped from the definition (instead of dropping it on every use site 
using a raw type -- if you find yourself doing that a lot, the problem 
might be in the generic definition itself...).

There were a few other classes like that which were not public API that 
had problematic generic parameters (`PartitionKey` for example, as it 
added nothing).

--John


On 05/12/2023 23:45, Andy Goryachev wrote:
>
> Yeah, that one.  Probably for the better - very little probability of 
> uncovering any bugs, and too high of a probability of introducing a 
> regression.
>
> Same with "unnecessary cast or instanceof" and "redundant null 
> check".  "potential null pointer access" - there is probably some 
> non-zero value in having the warning, but very few people would welcom 
> @SuppressWarning where it isn't an issue.
>
> You are right though - there are more important issues to be resolved 
> - 3,876 to be exact, see
>
> https://bugs.openjdk.org/issues/?jql=component%20in%20(javafx)%20and%20status%20in%20(open%2C%20new%2C%20%22In%20Progress%22)%20and%20labels%20not%20in%20(okToLeaveInJI)
>
> -andy
>
> *From: *John Hendrikx <john.hendrikx at gmail.com>
> *Date: *Tuesday, December 5, 2023 at 14:17
> *To: *Andy Goryachev <andy.goryachev at oracle.com>, 
> openjfx-dev at openjdk.org <openjfx-dev at openjdk.org>
> *Subject: *[External] : Re: eclipse warnings
>
> This one for example: https://github.com/openjdk/jfx/pull/1095 
> <https://urldefense.com/v3/__https:/github.com/openjdk/jfx/pull/1095__;!!ACWV5N9M2RV99hQ!NRHVOIJ65hopuPgetAT7stNzW3jh1BY4ZqHVTLuY3XHIQ72la7gG7TGgT_j0R9q_Gf3_l8RDUswG6IZijoq5ZLvZ3DXl$>
>
> It was auto closed, and at this point probably has many merge 
> conflicts, which is why I let it go closed.
>
> --John
>
> On 05/12/2023 20:27, Andy Goryachev wrote:
>
> > I did many warning fixes, and there are PR's outstanding with warning 
> fixes, but they're not getting reviewed.
>
> Are they still in Draft?
>
> https://github.com/openjdk/jfx/pulls?q=is%3Aopen+is%3Apr+label%3Arfr 
> <https://urldefense.com/v3/__https:/github.com/openjdk/jfx/pulls?q=is*3Aopen*is*3Apr*label*3Arfr__;JSslKyU!!ACWV5N9M2RV99hQ!NRHVOIJ65hopuPgetAT7stNzW3jh1BY4ZqHVTLuY3XHIQ72la7gG7TGgT_j0R9q_Gf3_l8RDUswG6IZijoq5ZP_iqZdL$>
>
> -andy
>
> *From: *openjfx-dev <openjfx-dev-retn at openjdk.org> 
> <mailto:openjfx-dev-retn at openjdk.org> on behalf of John Hendrikx 
> <john.hendrikx at gmail.com> <mailto:john.hendrikx at gmail.com>
> *Date: *Tuesday, December 5, 2023 at 03:16
> *To: *openjfx-dev at openjdk.org <openjfx-dev at openjdk.org> 
> <mailto:openjfx-dev at openjdk.org>
> *Subject: *Re: eclipse warnings
>
> IMHO, there is no capacity for this.
>
> I did many warning fixes, and there are PR's outstanding with warning 
> fixes, but they're not getting reviewed.
>
> There are other PR's outstanding that are more valuable, but are not 
> getting reviewed.
>
> I feel we need to fix that first before we can endulge in warning fixes.
>
> As for the potential null pointer access, it's often a false positive; 
> static analyzers have a hard time determining if a loop is entered at 
> least once (or an if in that loop) and so will warn that a local can 
> maybe be null if it was initalized inside a loop.
>
> --John
>
> On 04/12/2023 17:34, Andy Goryachev wrote:
>
>
> Dear colleagues:
>
> Imported the openjfx project into another workspace with a more 
> stringent error checking and discovered a few issues:
>
>  1. potential null pointer access: 295
>  2. unnecessary cast or instanceof: 190
>  3. redundant null check: 61
>
> Do we want to clean these up?
>
> -andy
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20231206/20c758d6/attachment-0001.htm>