refactor(interfaces): Make type predicates more robust (#9150)

* refactor(interfaces): Use typeof ... === 'function' to test for methods

  Testing for

      'name' in object

  or

      obj.name !== undefined

  only checks for the existence of the property (and in the latter
  case that the property is not set to undefined).  That's fine if
  the interface specifies a property of indeterminate type, but in
  the usual case that the interface member is a method we can do
  one better and check to make sure the property's value is
  callable.

* refactor(interfaces): Always check obj is not null/undefined

  Since most type predicates take an argument of type any but then
  check for the existence of certain properties, explicitly check
  that the argument is not null or undefined (or check implicitly
  by calling another type predicate that does so first, which
  necessitates adding a few casts because tsc infers the type of
  the argument too narrowly).

* fix(interfaces): Add missing check to hasBubble type predicate

  This appears to have inadvertently been omitted in PR #9004.

* fix(interfaces): Fix misplaced typeof

* fix: Fix typos in JSDocs

* fix(tests): Make Mocks conform to corresponding interfaces

  Introduce a new MockFocusable, and add methods to MockIcon,
  MockBubbleIcon and MockComment, so that they fulfil the
  IFocusableNode, IIcon, IHasBubble and ICommentIcon interfaces
  respectively.

* chore(tests): Add assertions verifying mocks conform to predicates

  Add (test) runtime assertions that:

  - isFocusableNode(MockFocusable) returns true
  - isIcon(MockIcon) returns true
  - hasBubble(MockBubbleIcon) returns true
  - isCommentIcon(MockCommentIcon) returns true

  (The latter is currently failing because Blockly is undefined when
  isCommentIcon calls the MockCommentIcon's getType method.)

* fix(tests): Don't rely on Blockly being set in Mock methods

  For some reason the global Blockly binding is not visible at the
  time when isCommentIcon calls MockCommentIcon's getType method,
  and presumably this problem would apply to getBubbleSize too,
  so directly import the required items.

* refactor(tests): Make MockCommentIcon a MockBubbleIcon

  This slightly simplifies it and makes it less likely to accidentally
  stop conforming to IHasBubble.

* fix(interfaces): Fix incorrect check in isSelectable

  Fix an error which caused ISelectable instances to fail
  isSelectable() checks, one of the results of which is that
  Blockly.common.getSelected() would generally return null.

  Whoops!

core/interfaces/i_autohideable.ts

@@ -23,5 +23,5 @@ export interface IAutoHideable extends IComponent {
 
 /** Returns true if the given object is autohideable. */
 export function isAutoHideable(obj: any): obj is IAutoHideable {
-  return obj.autoHide !== undefined;
+  return obj && typeof obj.autoHide === 'function';
 }

core/interfaces/i_comment_icon.ts

@@ -31,17 +31,17 @@ export interface ICommentIcon extends IIcon, IHasBubble, ISerializable {
 }
 
 /** Checks whether the given object is an ICommentIcon. */
-export function isCommentIcon(obj: object): obj is ICommentIcon {
+export function isCommentIcon(obj: any): obj is ICommentIcon {
   return (
     isIcon(obj) &&
     hasBubble(obj) &&
     isSerializable(obj) &&
-    (obj as any)['setText'] !== undefined &&
-    (obj as any)['getText'] !== undefined &&
-    (obj as any)['setBubbleSize'] !== undefined &&
-    (obj as any)['getBubbleSize'] !== undefined &&
-    (obj as any)['setBubbleLocation'] !== undefined &&
-    (obj as any)['getBubbleLocation'] !== undefined &&
+    typeof (obj as any).setText === 'function' &&
+    typeof (obj as any).getText === 'function' &&
+    typeof (obj as any).setBubbleSize === 'function' &&
+    typeof (obj as any).getBubbleSize === 'function' &&
+    typeof (obj as any).setBubbleLocation === 'function' &&
+    typeof (obj as any).getBubbleLocation === 'function' &&
     obj.getType() === IconType.COMMENT
   );
 }

core/interfaces/i_copyable.ts

@@ -35,5 +35,5 @@ export type ICopyData = ICopyable.ICopyData;
 
 /** @returns true if the given object is an ICopyable. */
 export function isCopyable(obj: any): obj is ICopyable<ICopyData> {
-  return obj.toCopyData !== undefined;
+  return obj && typeof obj.toCopyData === 'function';
 }

core/interfaces/i_deletable.ts

@@ -27,8 +27,9 @@ export interface IDeletable {
 /** Returns whether the given object is an IDeletable. */
 export function isDeletable(obj: any): obj is IDeletable {
   return (
-    obj['isDeletable'] !== undefined &&
-    obj['dispose'] !== undefined &&
-    obj['setDeleteStyle'] !== undefined
+    obj &&
+    typeof obj.isDeletable === 'function' &&
+    typeof obj.dispose === 'function' &&
+    typeof obj.setDeleteStyle === 'function'
   );
 }

core/interfaces/i_draggable.ts

@@ -62,11 +62,12 @@ export interface IDragStrategy {
 /** Returns whether the given object is an IDraggable or not. */
 export function isDraggable(obj: any): obj is IDraggable {
   return (
-    obj.getRelativeToSurfaceXY !== undefined &&
-    obj.isMovable !== undefined &&
-    obj.startDrag !== undefined &&
-    obj.drag !== undefined &&
-    obj.endDrag !== undefined &&
-    obj.revertDrag !== undefined
+    obj &&
+    typeof obj.getRelativeToSurfaceXY === 'function' &&
+    typeof obj.isMovable === 'function' &&
+    typeof obj.startDrag === 'function' &&
+    typeof obj.drag === 'function' &&
+    typeof obj.endDrag === 'function' &&
+    typeof obj.revertDrag === 'function'
   );
 }

core/interfaces/i_focusable_node.ts

@@ -102,16 +102,16 @@ export interface IFocusableNode {
  * Determines whether the provided object fulfills the contract of
  * IFocusableNode.
  *
- * @param object The object to test.
+ * @param obj The object to test.
  * @returns Whether the provided object can be used as an IFocusableNode.
  */
-export function isFocusableNode(object: any | null): object is IFocusableNode {
+export function isFocusableNode(obj: any): obj is IFocusableNode {
   return (
-    object &&
-    'getFocusableElement' in object &&
-    'getFocusableTree' in object &&
-    'onNodeFocus' in object &&
-    'onNodeBlur' in object &&
-    'canBeFocused' in object
+    obj &&
+    typeof obj.getFocusableElement === 'function' &&
+    typeof obj.getFocusableTree === 'function' &&
+    typeof obj.onNodeFocus === 'function' &&
+    typeof obj.onNodeBlur === 'function' &&
+    typeof obj.canBeFocused === 'function'
   );
 }

core/interfaces/i_focusable_tree.ts

@@ -128,17 +128,17 @@ export interface IFocusableTree {
  * Determines whether the provided object fulfills the contract of
  * IFocusableTree.
  *
- * @param object The object to test.
+ * @param obj The object to test.
  * @returns Whether the provided object can be used as an IFocusableTree.
  */
-export function isFocusableTree(object: any | null): object is IFocusableTree {
+export function isFocusableTree(obj: any): obj is IFocusableTree {
   return (
-    object &&
-    'getRootFocusableNode' in object &&
-    'getRestoredFocusableNode' in object &&
-    'getNestedTrees' in object &&
-    'lookUpFocusableNode' in object &&
-    'onTreeFocus' in object &&
-    'onTreeBlur' in object
+    obj &&
+    typeof obj.getRootFocusableNode === 'function' &&
+    typeof obj.getRestoredFocusableNode === 'function' &&
+    typeof obj.getNestedTrees === 'function' &&
+    typeof obj.lookUpFocusableNode === 'function' &&
+    typeof obj.onTreeFocus === 'function' &&
+    typeof obj.onTreeBlur === 'function'
   );
 }

core/interfaces/i_has_bubble.ts

@@ -30,6 +30,8 @@ export interface IHasBubble {
 /** Type guard that checks whether the given object is a IHasBubble. */
 export function hasBubble(obj: any): obj is IHasBubble {
   return (
-    obj.bubbleIsVisible !== undefined && obj.setBubbleVisible !== undefined
+    typeof obj.bubbleIsVisible === 'function' &&
+    typeof obj.setBubbleVisible === 'function' &&
+    typeof obj.getBubble === 'function'
   );
 }

core/interfaces/i_icon.ts

@@ -98,19 +98,19 @@ export interface IIcon extends IFocusableNode {
 /** Type guard that checks whether the given object is an IIcon. */
 export function isIcon(obj: any): obj is IIcon {
   return (
-    obj.getType !== undefined &&
-    obj.initView !== undefined &&
-    obj.dispose !== undefined &&
-    obj.getWeight !== undefined &&
-    obj.getSize !== undefined &&
-    obj.applyColour !== undefined &&
-    obj.hideForInsertionMarker !== undefined &&
-    obj.updateEditable !== undefined &&
-    obj.updateCollapsed !== undefined &&
-    obj.isShownWhenCollapsed !== undefined &&
-    obj.setOffsetInBlock !== undefined &&
-    obj.onLocationChange !== undefined &&
-    obj.onClick !== undefined &&
-    isFocusableNode(obj)
+    isFocusableNode(obj) &&
+    typeof (obj as IIcon).getType === 'function' &&
+    typeof (obj as IIcon).initView === 'function' &&
+    typeof (obj as IIcon).dispose === 'function' &&
+    typeof (obj as IIcon).getWeight === 'function' &&
+    typeof (obj as IIcon).getSize === 'function' &&
+    typeof (obj as IIcon).applyColour === 'function' &&
+    typeof (obj as IIcon).hideForInsertionMarker === 'function' &&
+    typeof (obj as IIcon).updateEditable === 'function' &&
+    typeof (obj as IIcon).updateCollapsed === 'function' &&
+    typeof (obj as IIcon).isShownWhenCollapsed === 'function' &&
+    typeof (obj as IIcon).setOffsetInBlock === 'function' &&
+    typeof (obj as IIcon).onLocationChange === 'function' &&
+    typeof (obj as IIcon).onClick === 'function'
   );
 }

core/interfaces/i_legacy_procedure_blocks.ts

@@ -28,9 +28,9 @@ export interface LegacyProcedureDefBlock {
 
 /** @internal */
 export function isLegacyProcedureDefBlock(
-  block: object,
-): block is LegacyProcedureDefBlock {
-  return (block as any).getProcedureDef !== undefined;
+  obj: any,
+): obj is LegacyProcedureDefBlock {
+  return obj && typeof obj.getProcedureDef === 'function';
 }
 
 /** @internal */
@@ -41,10 +41,11 @@ export interface LegacyProcedureCallBlock {
 
 /** @internal */
 export function isLegacyProcedureCallBlock(
-  block: object,
-): block is LegacyProcedureCallBlock {
+  obj: any,
+): obj is LegacyProcedureCallBlock {
   return (
-    (block as any).getProcedureCall !== undefined &&
-    (block as any).renameProcedure !== undefined
+    obj &&
+    typeof obj.getProcedureCall === 'function' &&
+    typeof obj.renameProcedure === 'function'
   );
 }

core/interfaces/i_observable.ts

@@ -20,5 +20,9 @@ export interface IObservable {
  * @internal
  */
 export function isObservable(obj: any): obj is IObservable {
-  return obj.startPublishing !== undefined && obj.stopPublishing !== undefined;
+  return (
+    obj &&
+    typeof obj.startPublishing === 'function' &&
+    typeof obj.stopPublishing === 'function'
+  );
 }

core/interfaces/i_paster.ts

@@ -21,5 +21,5 @@ export interface IPaster<U extends ICopyData, T extends ICopyable<U>> {
 export function isPaster(
   obj: any,
 ): obj is IPaster<ICopyData, ICopyable<ICopyData>> {
-  return obj.paste !== undefined;
+  return obj && typeof obj.paste === 'function';
 }

core/interfaces/i_procedure_block.ts

@@ -20,9 +20,10 @@ export interface IProcedureBlock {
 export function isProcedureBlock(
   block: Block | IProcedureBlock,
 ): block is IProcedureBlock {
+  block = block as IProcedureBlock;
   return (
-    (block as IProcedureBlock).getProcedureModel !== undefined &&
-    (block as IProcedureBlock).doProcedureUpdate !== undefined &&
-    (block as IProcedureBlock).isProcedureDef !== undefined
+    typeof block.getProcedureModel === 'function' &&
+    typeof block.doProcedureUpdate === 'function' &&
+    typeof block.isProcedureDef === 'function'
   );
 }

core/interfaces/i_rendered_element.ts

@@ -15,5 +15,5 @@ export interface IRenderedElement {
  * @returns True if the given object is an IRenderedElement.
  */
 export function isRenderedElement(obj: any): obj is IRenderedElement {
-  return obj['getSvgRoot'] !== undefined;
+  return obj && typeof obj.getSvgRoot === 'function';
 }

core/interfaces/i_selectable.ts

@@ -30,12 +30,12 @@ export interface ISelectable extends IFocusableNode {
 }
 
 /** Checks whether the given object is an ISelectable. */
-export function isSelectable(obj: object): obj is ISelectable {
+export function isSelectable(obj: any): obj is ISelectable {
   return (
-    typeof (obj as any).id === 'string' &&
-    (obj as any).workspace !== undefined &&
-    (obj as any).select !== undefined &&
-    (obj as any).unselect !== undefined &&
-    isFocusableNode(obj)
+    isFocusableNode(obj) &&
+    typeof (obj as ISelectable).id === 'string' &&
+    typeof (obj as ISelectable).workspace === 'object' &&
+    typeof (obj as ISelectable).select === 'function' &&
+    typeof (obj as ISelectable).unselect === 'function'
   );
 }

core/interfaces/i_serializable.ts

@@ -24,5 +24,9 @@ export interface ISerializable {
 
 /** Type guard that checks whether the given object is a ISerializable. */
 export function isSerializable(obj: any): obj is ISerializable {
-  return obj.saveState !== undefined && obj.loadState !== undefined;
+  return (
+    obj &&
+    typeof obj.saveState === 'function' &&
+    typeof obj.loadState === 'function'
+  );
 }

tests/mocha/block_test.js

@@ -7,7 +7,10 @@
 import {ConnectionType} from '../../build/src/core/connection_type.js';
 import {EventType} from '../../build/src/core/events/type.js';
 import * as eventUtils from '../../build/src/core/events/utils.js';
+import {IconType} from '../../build/src/core/icons/icon_types.js';
 import {EndRowInput} from '../../build/src/core/inputs/end_row_input.js';
+import {isCommentIcon} from '../../build/src/core/interfaces/i_comment_icon.js';
+import {Size} from '../../build/src/core/utils/size.js';
 import {assert} from '../../node_modules/chai/chai.js';
 import {createRenderedBlock} from './test_helpers/block_definitions.js';
 import {
@@ -1426,9 +1429,9 @@ suite('Blocks', function () {
     });
 
     suite('Constructing registered comment classes', function () {
-      class MockComment extends MockIcon {
+      class MockComment extends MockBubbleIcon {
         getType() {
-          return Blockly.icons.IconType.COMMENT;
+          return IconType.COMMENT;
         }
 
         setText() {}
@@ -1440,19 +1443,13 @@ suite('Blocks', function () {
         setBubbleSize() {}
 
         getBubbleSize() {
-          return Blockly.utils.Size(0, 0);
+          return Size(0, 0);
         }
 
         setBubbleLocation() {}
 
         getBubbleLocation() {}
 
-        bubbleIsVisible() {
-          return true;
-        }
-
-        setBubbleVisible() {}
-
         saveState() {
           return {};
         }
@@ -1460,6 +1457,10 @@ suite('Blocks', function () {
         loadState() {}
       }
 
+      if (!isCommentIcon(new MockComment())) {
+        throw new TypeError('MockComment not an ICommentIcon');
+      }
+
       setup(function () {
         this.workspace = Blockly.inject('blocklyDiv', {});
 

tests/mocha/test_helpers/icon_mocks.js

@@ -4,7 +4,24 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-export class MockIcon {
+import {isFocusableNode} from '../../../build/src/core/interfaces/i_focusable_node.js';
+import {hasBubble} from '../../../build/src/core/interfaces/i_has_bubble.js';
+import {isIcon} from '../../../build/src/core/interfaces/i_icon.js';
+import {isSerializable} from '../../../build/src/core/interfaces/i_serializable.js';
+
+export class MockFocusable {
+  getFocusableElement() {}
+  getFocusableTree() {}
+  onNodeFocus() {}
+  onNodeBlur() {}
+  canBeFocused() {}
+}
+
+if (!isFocusableNode(new MockFocusable())) {
+  throw new TypeError('MockFocusable not an IFocuableNode');
+}
+
+export class MockIcon extends MockFocusable {
   getType() {
     return new Blockly.icons.IconType('mock icon');
   }
@@ -52,6 +69,10 @@ export class MockIcon {
   }
 }
 
+if (!isIcon(new MockIcon())) {
+  throw new TypeError('MockIcon not an IIcon');
+}
+
 export class MockSerializableIcon extends MockIcon {
   constructor() {
     super();
@@ -75,6 +96,10 @@ export class MockSerializableIcon extends MockIcon {
   }
 }
 
+if (!isSerializable(new MockSerializableIcon())) {
+  throw new TypeError('MockSerializableIcon not an ISerializable');
+}
+
 export class MockBubbleIcon extends MockIcon {
   constructor() {
     super();
@@ -94,4 +119,12 @@ export class MockBubbleIcon extends MockIcon {
   setBubbleVisible(visible) {
     this.visible = visible;
   }
+
+  getBubble() {
+    return null;
+  }
+}
+
+if (!hasBubble(new MockBubbleIcon())) {
+  throw new TypeError('MockBubbleIcon not an IHasBubble');
 }