Most specific method in diagnostic generation for overload resolution

[B. Blaser]

On 26 September 2017 at 15:40, B. Blaser <bsrbnd at gmail.com> wrote:
> Hi Vicente,
>
> Thanks for looking at this.
>
> On 26 September 2017 at 02:14, Vicente Romero <vicente.romero at oracle.com> wrote:
>> Hi Bernard,
>>
>> Thanks for the patch. I was playing a bit with it and made some
>> modifications to it, my changes are just in the put method of
>> MostSpecificMap.
>>
>> in your previous version you were doing:
>>
>> +                      new LinkedList<>(keySet()).stream()
>> +                            .filter( s -> !s.equals(c.sym) )
>> +                            .filter( s -> c.sym.overrides(s,
>> (TypeSymbol)s.owner, types, false) )
>> +                            .forEach( s -> remove(s) );
>>
>> by creating a brand new LinkedList my impression is that any removal of a
>> key would happen in the linked list, not in the original map so the first
>> statement was a no-op.
>
> "s -> remove(s)" is a closure on "MostSpecificMap.put()".
> So, "remove(s)" is applied on the map (if all goes well), which is
> what we expect, I think.
>
> I thought that duplicating the *keyset* was necessary because we can't
> modify the map ("remove(s)") while iterating on it, but we have to
> verify this.
>
>> Also as symbols are unique in javac you can safely
>> use the "==" operator for comparison.
>
> Thanks for the confirmation (I wasn't enough sure to safely use "!=") !
> Note that this is "!=", not "==" as you used in:
>
> +                    if (!keySet().stream()
> +                            .filter(s -> s == c.sym)
> +                            .anyMatch(s -> s.overrides(c.sym,
> (TypeSymbol)c.sym.owner, types, false))) {
> +                        put(c.sym, c.details);
>
> Here, we would like to add a candidate if no method overrides it,
> *except* itself.
>
>> Does my version captures what you intended?
>
> We just need to be sure that removing elements in the map is possible
> while iterating on it, as explained above.
>
>> In addition to all this I think that we should consider if List is the best
>> data structure to store the candidates. It sounds like it could be a map to
>> avoid duplicates, probably this won't be a great gain but there is no point
>> on storing duplicates when we can check for them in O(1) time,
>
> I've already tried not to duplicate candidates but "mapCandidates()"
> was already using a "LinkedHashMap" and the rest of the diagnostic
> generation is built above that.
>
> Notice also that while "c.sym" could be added twice, the corresponding
> "c.details" would be different.
>
> So, using another structure would probably be more complicated I
> think. And while I'm very concerned about performances, I think this
> wouldn't be a great gain here.

To give a concrete example not fully related to our diagnostic problem
but involving duplicates in the candidates list, see
"test/tools/javac/Diagnostics/compressed/T8020286.java" :

class T8020286 {
   void m(String s) { }
   void m(Integer i, String s) { }
   void test() {
       m(1, 1);
       m(1);
   }
}

Here, "MethodResolutionContext.addInapplicableCandidate()" is applied
twice with the same symbol but with different "details". The same
symbol appears then twice in the "candidates" list.

I'm honestly not sure if this is really expected but changing it would
probably have side-effects where the list is used.

But in "InapplicableSymbolsError.mapCandidates()" we store them in a
map as it was done before and in the same order to preserve the same
behavior.

Bernard

> Do you agree?
>
> Thanks,
> Bernard
>
>> Thanks,
>> Vicente