Fix signatures of JavaMap which can return undefined #67

sharwell · 2016-10-10T11:21:52Z

📝 Strict null checks are currently disabled, but we're working to change that. See #26.

BurtHarris

While I would be OK as written, I suggest some changes.

BurtHarris · 2016-10-10T17:44:55Z

src/atn/PredictionContext.ts

@@ -248,7 +248,7 @@ export abstract class PredictionContext implements Equatable {
 			return context;
 		}

-		let existing: PredictionContext =  visited.get(context);
+		let existing: PredictionContext | undefined =  visited.get(context);


I think it cleaner to simply drop the type specification for the variable existing and allow type inference to drive. It seems particularly odd in juxtaposition with the following line comparing it against null. I think it's a case of less is better, so my personal preference would be:

let existing = visited.get(context); if (existing) { return existing; } existing = contextCache.get(context); if (existing) { visited.put(context, existing); return existing; }

BurtHarris · 2016-10-10T17:50:06Z

src/misc/Array2DHashMap.ts

@@ -50,7 +50,7 @@ class MapKeyEqualityComparator<K, V> implements EqualityComparator<[K, V]> {
 }

 export class Array2DHashMap<K, V> implements JavaMap<K, V> {
-	private backingStore: Array2DHashSet<[K, V]>;
+	private backingStore: Array2DHashSet<[K, V | undefined]>;


This union type here seems more than a little hard to understand. We would never actually store a [key, undefined] in here, right?

I think it might be substantially clarifying to define a type alias type Bucket = [K, V]; then in places needed cast the temporary bucket wrapping a key only with as Bucket. Perhaps even better: define an internal getBucket(key) that centralizes all cases currently needing this substituting this.backingStore.get([key,value]) with this.getBucket(key);

P.S., I think we should consider having Array2DHashMap implement JavaScript's Map as well as JavaMap. As I remember, I couldn't do that for Set vs JavaSet due to a return-type conflict, but with Map, I think it is possible. But it's probably best to do this as a separate, perhaps low-priority, change.

I don't want to resort to as T when there's an approach available that leverages full type inference. However, I also don't want the code to be confusing to readers. I think the solution here is a combination of a type alias and documentation explaining why the type alias includes | undefined in the value type.

Rather than type Bucket = ..., perhaps it would it help to define an non-exported class Bucket that hid the 'undefined'' in the constructor parameters would make it clearer. We could even combine that with functionality currently in the comparator, that only the key matters for equals/hash.

I think you'll like the approach I finally implemented. 😄

BurtHarris · 2016-10-10T17:51:27Z

src/atn/PredictionContext.ts

@@ -567,7 +567,7 @@ class ArrayPredictionContext extends PredictionContext {
 			throw "Appending a tree suffix is not yet supported.";
 		}

-		let result: PredictionContext = visited.get(context);
+		let result: PredictionContext | undefined = visited.get(context);


Same comment as above...

BurtHarris · 2016-10-10T17:51:38Z

src/atn/PredictionContextCache.ts

@@ -63,7 +63,7 @@ export class PredictionContextCache {
            return context;
        }

-        let result: PredictionContext =  this.contexts.get(context);
+        let result: PredictionContext | undefined =  this.contexts.get(context);


Same comment as above...

BurtHarris · 2016-10-10T17:51:50Z

src/atn/PredictionContextCache.ts

@@ -78,7 +78,7 @@ export class PredictionContextCache {
        }

        let operands: PredictionContextCache.PredictionContextAndInt =  new PredictionContextCache.PredictionContextAndInt(context, invokingState);
-        let result: PredictionContext =  this.childContexts.get(operands);
+        let result: PredictionContext | undefined =  this.childContexts.get(operands);


Same comment as above...

BurtHarris · 2016-10-10T18:11:08Z

src/misc/Array2DHashMap.ts

@@ -61,7 +61,7 @@ export class Array2DHashMap<K, V> implements JavaMap<K, V> {
 	}

 	containsKey(key: K): boolean {
-		return this.backingStore.contains([key, null]);
+		return this.backingStore.contains([key, undefined]);


I almost prefer the version where this is null.

📝 This is just omitted in the updated version

BurtHarris · 2016-10-10T18:21:12Z

src/misc/Array2DHashMap.ts

@@ -61,7 +61,7 @@ export class Array2DHashMap<K, V> implements JavaMap<K, V> {
 	}

 	containsKey(key: K): boolean {
-		return this.backingStore.contains([key, null]);
+		return this.backingStore.contains([key, undefined]);


I almost prefered the version where this is null. But if we follow the approach I suggested above, this might be

return this.backingStore.contains([key, null] as Bucket);

📝 This is just omitted in the updated version

BurtHarris · 2016-10-10T18:23:00Z

src/misc/Array2DHashMap.ts

-	put(key: K, value: V): V {
-		let element: [K, V] = this.backingStore.get([key, value]);
-		let result: V;
+	put(key: K, value: V): V | undefined {


Why would this ever return undefined?

➡️ put returns the value previously associated with the specified key. If no value was already in the map for this key, the method returns undefined.

BurtHarris · 2016-10-10T18:29:23Z

src/misc/Array2DHashMap.ts

-	put(key: K, value: V): V {
-		let element: [K, V] = this.backingStore.get([key, value]);
-		let result: V;
+	put(key: K, value: V): V | undefined {


That this might return undefined seems quite weird to me, but I guess that's what happens if the element key wasn't previously present in map.

We're doing something a bit wierd defining interface JavaMap, but then adopting it to return undefined rather than null as a concession to JavaScript.

BurtHarris · 2016-10-10T18:39:04Z

src/misc/Array2DHashMap.ts

-		let element: [K, V] = this.backingStore.get([key, value]);
-		let result: V;
+	putIfAbsent(key: K, value: V): V | undefined {
+		let element: [K, V | undefined] = this.backingStore.get([key, value]);


This is the one that convinced me it was confusing. While I understand the semantics implied by the comparitor ignores the value part of the bucket, that's non-local information that makes the behavior of this function hard to understand by examining only it's body.

Fix signatures of JavaMap which can return undefined

88f5470

sharwell added bug code review labels Oct 10, 2016

sharwell assigned BurtHarris Oct 10, 2016

BurtHarris requested changes Oct 10, 2016

View reviewed changes

BurtHarris assigned sharwell and unassigned BurtHarris Oct 10, 2016

sharwell added in progress and removed code review labels Oct 10, 2016

sharwell added 2 commits October 10, 2016 16:54

Use type inference in several additional places

c5a3513

Use an improved Bucket type for Array2DHashMap

2d0d368

sharwell added code review and removed in progress labels Oct 10, 2016

sharwell assigned BurtHarris and unassigned sharwell Oct 10, 2016

BurtHarris merged commit c465469 into master Oct 10, 2016

BurtHarris deleted the javamap-undefined branch October 10, 2016 22:12

BurtHarris added fixed and removed code review labels Oct 10, 2016

BurtHarris removed their assignment Oct 10, 2016

sharwell added needs comment review and removed needs comment review labels Oct 21, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix signatures of JavaMap which can return undefined #67

Fix signatures of JavaMap which can return undefined #67

sharwell commented Oct 10, 2016

BurtHarris left a comment

BurtHarris Oct 10, 2016

BurtHarris Oct 10, 2016 •

edited

Loading

BurtHarris Oct 10, 2016

sharwell Oct 10, 2016 •

edited

Loading

BurtHarris Oct 10, 2016 •

edited

Loading

sharwell Oct 10, 2016

BurtHarris Oct 10, 2016

BurtHarris Oct 10, 2016

BurtHarris Oct 10, 2016

BurtHarris Oct 10, 2016

sharwell Oct 10, 2016

BurtHarris Oct 10, 2016

sharwell Oct 10, 2016

BurtHarris Oct 10, 2016

sharwell Oct 10, 2016

BurtHarris Oct 10, 2016

BurtHarris Oct 10, 2016

Fix signatures of JavaMap which can return undefined #67

Fix signatures of JavaMap which can return undefined #67

Conversation

sharwell commented Oct 10, 2016

BurtHarris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BurtHarris Oct 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell Oct 10, 2016 • edited Loading

Choose a reason for hiding this comment

BurtHarris Oct 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BurtHarris Oct 10, 2016 •

edited

Loading

sharwell Oct 10, 2016 •

edited

Loading

BurtHarris Oct 10, 2016 •

edited

Loading