19

The following class has an inner class called Entry. This code will not compile in Java 8 as the compiler assumes the Entry referenced within the double curly brace initializer is of type Map.Entry and not Scope.Entry. This code compiles in previous versions (at least 6 and 7) of the JDK but is broken in JDK 8. My question is "why?" Map.Entry is not imported in this class, so there is no reason for the compiler to assume that the value is of type Map.Entry. Is there some implicit scope being brought in or something for anonymous classes?

Error:

scope/Scope.java:23: error: incompatible types: scope.Scope.Entry cannot be converted to java.util.Map.Entry for (final Entry entry : entries) {
scope/Scope.java:22: error: cannot find symbol put(entry.getName(), entry);

Example Code:

package scope;

import java.util.HashMap;
import java.util.HashSet;
import java.util.Set;

public class Scope {

    public static class Entry<T> {
        public String getName() {
            return "Scope";
        }
    }

    public static void main(String[] args) {
        final Set<Entry> entries = new HashSet<>();

        new HashMap<String, Entry>() {{
            // Why does the Java 8 compiler assume this is a Map.Entry
            // as it is not imported? 
            for (final Entry entry : entries) {
                put(entry.getName(), entry);
            }
        }};
    }
}
Floegipoky
  • 3,087
  • 1
  • 31
  • 47
Briggs
  • 639
  • 5
  • 13
  • I did not even know this double-curly-brace notation... is this documented somewhere? – tobias_k Nov 13 '14 at 16:21
  • 5
    @tobias_k It's not really double curly braces. There's a pair of braces to create an anonymous inner class and another pair for an instance initializer block. Some people like to format it as double curly braces but it has no special meaning really. – biziclop Nov 13 '14 at 16:23
  • @biziclop In this case, I did not know the instance initializer block. Thanks for giving it a name! I understand that this is similar to `static {...}` but for instances and it's always called even before the/each constructor? – tobias_k Nov 13 '14 at 16:30
  • 2
    @tobias_k Yes, in practice the initializer blocks are copied to the start of every constructor. (If there are multiple blocks, they are executed in the order in which they appear in the source code.) – biziclop Nov 13 '14 at 16:38
  • 1
    @Floegipoky Yeah, fully qualifying works. It's just one of those breaking changes that happened from legacy code I wrote in a system that is still running and causing an issue when upgrading to JDK 8. Just strange behavior, IMHO. I use to do this a lot because I thought it was elegant and encapsulated the initialization. That's all. So, I suppose I should report it if people think it's a bug. – Briggs Nov 13 '14 at 17:37
  • The for-statement is in an implementation of `HashMap` which implements the `Map` interface where `Map.Entry` is defined. Could this be the reason? However this does not explain why it worked in Java 7. – eee Nov 13 '14 at 18:32
  • 1
    @eee: The issue is, that both classes (Scope.Entry and Map.Entry) are within scope and visible. When two declarations with the same name is within scope (this may also apply to other declarations like variables, method names etc.), the Java Language Specification determines precedence of the different declarations in the section on "shadowing". Here, the wording is quite vague, but as I understand the specification, the behaviour of the JDK7 compiler must be correct. The unqualified usage of "Entry" should refer to the Scope.Entry class. – jarnbjo Nov 13 '14 at 18:50
  • 5
    By the way, the term "curly braces" is a pet peeve of mine. They're just "braces". – Floegipoky Nov 13 '14 at 19:37
  • @Floegipoky http://en.wikipedia.org/wiki/Bracket#Curly_brackets Looks like they're actually 'Curly Brackets' – Patrick Nov 13 '14 at 19:57
  • 2
    @Patrick The very first words of that section: `"Curly brackets—also called braces"` – Floegipoky Nov 13 '14 at 20:16
  • @Floegipoky As the section notes, it has become a not totally uncommon usage for 'brace' to be a synonym of 'bracket' by some (regardless of that being incorrect usage), which would then make 'brace' alone ambiguous. Should probably move this to english.stackexchange to question the appropriateness of 'curly brace' vs. 'brace' vs 'curly bracket', heh – Patrick Nov 13 '14 at 20:34
  • 3
    Use `entries.stream().collect(toMap(Entry::getName, e->e))` (with `Collectors.toMap` statically imported) instead of "double brace initialization", which sucks. – David Conrad Nov 13 '14 at 21:29
  • 2
    @DavidConrad Yeah, well, that's just, like, your opinion, man. Seriously though, like every facet of any language, it's fine to use in appropriate places if you understand what it does. As discussed [here](http://stackoverflow.com/q/924285/2517719), the performance impact is negligible for code generated by humans. – Floegipoky Nov 13 '14 at 22:48
  • 4
    The problem is that it captures a reference to the enclosing class, preventing it from being garbage collected as long as the map lives. Of course, that might not be bad in one spot, but are you going to stop and think about it every time before you instantiate a map like this? Is every developer on your project going to consider that before moving some code to a different class? Are they all going to check for the double brace idiom in a class before adding some massive state to it? I'd avoid this idiom. There are much better ways. – David Conrad Nov 13 '14 at 23:02
  • It’s irrelevant how little the overhead is. After all, this anti-pattern saves only a single writing of a variable name and a dot in the source code here (but adds four additional curly brackets, reducing the “advantage” even more). Given this hilariously little advantage, even the slightest overhead (and there is one) is unacceptable. – Holger Nov 14 '14 at 10:15
  • @DavidConrad Mind you, it isn't the double brace idiom itself that is the trouble, it's the fact that you created a non-static nested class. If you do it in a static context, there's no extra state captured. Of course you still created an extra class when there really was no need for one. – biziclop Nov 14 '14 at 10:15
  • @biziclop Yes, but again, what if you did it in a static context and another developer decided to refactor that code and move the creation of the map into another method? Are they always going to stop and think, "Wait, this is double brace, this class has a lot of state, better not move that there." And there are better ways. So just ban this idiom with the burning hatred of a million fiery suns. – David Conrad Nov 14 '14 at 21:13
  • @DavidConrad That might be a bit dramatic. These aren't `goto`s. There are certainly times it could eat memory to the point of being a problem, and you're right to make people aware of that, but I've never seen this practically be an app killer, even if slightly inefficient. It's clean to read, and I'm under the impression most modern programmers doing non-mission critical apps are leaning toward readability and maintainability over premature optimization. When compared to even a slightly misconfigured Hibernate or AOP, this seems like a relatively trivial thing to get fired up about. – Patrick Nov 20 '14 at 02:31
  • @Patrick But this is premature pessimization. In the app I work on, this could easily result in many megabytes of data being unintentionally retained (by putting this into a class that is handling our XML documents). Meanwhile, the alternatives are just as clean to read, easy to maintain, and don't have any of the drawbacks. – David Conrad Nov 20 '14 at 04:23
  • 2
    It's nice for everyone to have opinions and all of the usage of this idiom, but these don't answer the question as to the why the behavior existed in one version of the compiler over the other. I simply demonstrated the behavior in a concise manner. Please stay on topic. I marked the question as answered. – Briggs Nov 20 '14 at 15:44
  • 1
    @biziclop there is no outer `this` to capture, but there is the `entries` variable, over which the initializer iterates. And this is unavoidable; as soon as a doubly-curly-brace initializer uses non-constant values, it has to capture variables, hence will hold them during the entire lifetime of the collection or map. That’s doubling the necessary storage in the best case, i.e. when it is only about the same references, but will be more when the data is not just put in, but converted or used for calculations and even worse, not actually removed when it gets removed later-on… – Holger Sep 17 '18 at 09:07

2 Answers2

27

It's definitely not a bug, it's a side-effect of using double-brace initialization.

new HashMap<String, Entry>() {{
    for (final Entry entry : entries) {
        put(entry.getName(), entry);
    }
}};

This type of initialization is basically a clever way to abuse instance initialization blocks. It creates an anonymous subclass of HashMap with an initialization block, and then copies that block into the beginning of its default constructor before calling it. This subclass gives priority to the Entry in the scope of its parent, rather than the scope that it's nested in. This is explained by shadowing.

From 8.1.6. Class Body and Member Declarations

If C itself is a nested class, there may be definitions of the same kind (variable, method, or type) and name as m in enclosing scopes. (The scopes may be blocks, classes, or packages.) In all such cases, the member m declared in or inherited by C shadows (§6.4.1) the other definitions of the same kind and name. [emphasis mine]

Here, C is the anonymous inner class declared. Since it inherits from HashMap, java.util.Map.Entry shadows scope.Scope.Entry.

As for why it compiled as you wanted it to with previous versions, I have no idea. This behavior was present in those versions (the docs I referenced are from 7), so it shouldn't have worked. So maybe those versions are bugged.

Floegipoky
  • 3,087
  • 1
  • 31
  • 47
  • 1
    Where is it specified in the JLS that extending a class *declares* the inner classes of the super class? Extending a class gives you access to the class' inner classes for sure, but the declaration of the inner classes is done somewhere else. Or why do you interpret subclassing and declaration as equal? – jarnbjo Nov 13 '14 at 21:04
  • 1
    Yeah, now you're right :) I tried to find something similar in §6.4.1, where shadowing *actually* is specified, and in that paragraph there is no description of this situation. – jarnbjo Nov 13 '14 at 23:22
0

Scopes of type members and shadowing is a hard place for a compiler. There was/are number of bugs related to this - mostly about nested/inner/anonymous types. I can't find the one that is exactly about that issue but I know some that can be related to it. Here is the one with a case very similiar to this (type variable instead of enclosing type).

Concerning what specification says about shadowing there is also an issue. It has references to JLS and describes what is not ideal.

Stanislav Lukyanov
  • 2,147
  • 10
  • 20