Replace usage of Maps.newHashMapWithExpectedSize() with standard Java

[HannesWell]

In Java-19 there is even static factory method HashMap.newHashMap(int) that can be used as a drop-in replacement once Java-19 or later is required.
For now I calculate the necessary capacity explicitly using the formula: expected-size *4/3+1.

The change in XtextGeneratorTemplates also removes the usage of Maps.newHashMapWithExpectedSize() of many generated Activators, but I haven't changed them manually. I assume they should be regenerated, shouldn't they?

Part of #2975

[LorenzoBettini]

I think @szarnekow should review this, but personally, I'm not a big fan of such strange expressions for computing the size ;)

another note: Maps.of returns an immutable map, IIRC. Could this potentially lead to breakage if the current factory method returns a mutable list?

[HannesWell]

I think @szarnekow should review this, but personally, I'm not a big fan of such strange expressions for computing the size ;)

Is it about the expression or specifying the size at all?

If it is the latter, I can also remove it. If it is the former I'm fine to restrict this PR to the change of XtextGeneratorTemplates.xtend, which is 'required' for eclipse/mwe#287 (comment) or at least the changes that use only constants. I mainly did the others for completeness.
The remaining parts could then be replaced as soon as Java-21 is required using the method HashMap.newHashMap(int) introduced in Java-19 as mentioned in my initial message.

another note: Maps.of returns an immutable map, IIRC. Could this potentially lead to breakage if the current factory method returns a mutable list?

That's right. But I have only used it in two places where the map is passed as userData to EObjectDescription.create(). And if I have followed the code-path correctly that map is only read there.

[HannesWell]

I have now reverted the changes that require computation of the capacity. That can be replaced once the Java-19 HashMap.newHashMap(int) is available for Xtext.
The main change for this PR in XtextGeneratorTemplates.xtend is not affected by that.

[HannesWell]

Is there anything left here to land this?

[cdietrich]

i guess a rebase would be helpful.
and any cappa in the team that does not need to be spent on breaking upstream changes....

[HannesWell]

i guess a rebase would be helpful.

Rebased it and looked into the currently open issues and answered where I can think I can help something. :)

[szarnekow]

I'm reluctant to accept this change, since the map will now throw an NPE when queried with null as a key. Though that's a valid implementation of the Map interface, it doesn't follow the principle of least surprise, which would just be containsKey(null) == false and get(null) == null

[HannesWell]

In both cases the map is passed to org.eclipse.xtext.resource.EObjectDescription and only queried in

xtext/org.eclipse.xtext/src/org/eclipse/xtext/resource/EObjectDescription.java

Lines 89 to 94 in 2bce8b6

	@Override
	public String getUserData(String name) {
	if (userData==null)
	return null;
	return userData.get(name);
	}

The doc of that method already defines that the name argument must not be null:

xtext/org.eclipse.xtext/src/org/eclipse/xtext/resource/IEObjectDescription.java

Lines 49 to 54 in 2bce8b6

	/**
	* Access to specific user data.
	* @param key the user data key. May not be <code>null</code>. Unknown keys yield <code>null</code>.
	* @return the value. May be <code>null</code>.
	*/
	String getUserData(String key);

It was indeed not enforced before, but the new behavior would be 100% compliant with the specification of the method.

[szarnekow]

Understandable. Yet I prefer some simple guidelines in a code base, one of which would be to avoid the overly strict immutable collections. I've seen to many NPE when doing containsKey, indexOf or contains checks on these with a null in code paths that used to work great until they were refactored to the immutable collections.

[HannesWell]

Personally I think it is good that these immutable collections are strict with null elements since one is forced to be more careful in using null, which on the long run and in general is an improvement for a code base, even though it can be annoying in the beginning.

But since you prefer not to use it here I replaced these two usages of Map.of() with HashMap.

[szarnekow]

Curious: Why didn't you use Collections.singletonMap instead?
(No need to change this)

[HannesWell]

Curious: Why didn't you use Collections.singletonMap instead?

I'm not sure anymore. I think I just forgot it during the discussion and initially I probably assumed that Map.of() is more elegant while meeting the spec. of the methods.

(No need to change this)

Even if there is no need, I think that's the better solution and change it accordingly.

[github-actions]

Test Results

  6 460 files  ±0    6 460 suites  ±0   3h 12m 48s ⏱️ + 6m 42s
 43 240 tests ±0   42 657 ✅ ±0    583 💤 ±0  0 ❌ ±0 
170 100 runs   - 5  167 736 ✅ ±0  2 354 💤 ±0  9 ❌  - 3  1 🔥  - 2 

Results for commit 9ebd37b. ± Comparison against base commit 01fced3.

♻️ This comment has been updated with latest results.

[LorenzoBettini]

@HannesWell two things I don't understand about test results:

• there's no way to see the actual results? I can't seem to be able to click anything on the results
• if it works only for PR, then it won't work for committers, since we skip the building for our PRs and only build for pushes

[LorenzoBettini]

Of course, for the second item, we could change our design and also build PRs for committers, cc @cdietrich

[szarnekow]

@LorenzoBettini Does your comment on the other MR mean that the check result Some checks were not successful cannot be trusted?

[LorenzoBettini]

@LorenzoBettini Does your comment on the other MR mean that the check result Some checks were not successful cannot be trusted?

The "Build / Test Results" failing cannot be trusted: it fails even in the presence of flakes. We can only trust "Build / build..." jobs failing.

Unfortunately, the test results action cannot handle flaky tests currently.

[HannesWell]

Can this be considered for Xtext M3?

[szarnekow]

I'd be ok with merging für M3