Skip to content

Fix positional inference #777

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Jun 21, 2022
3 changes: 2 additions & 1 deletion src/core/commandRunner/CommandRunner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import * as vscode from "vscode";
import { ActionType } from "../../actions/actions.types";
import { OutdatedExtensionError } from "../../errors";
import processTargets from "../../processTargets";
import isTesting from "../../testUtil/isTesting";
import { Graph, ProcessedTargetsContext } from "../../typings/Types";
import { isString } from "../../util/type";
import { canonicalizeAndValidateCommand } from "../commandVersionUpgrades/canonicalizeAndValidateCommand";
Expand Down Expand Up @@ -146,7 +147,7 @@ export default class CommandRunner {
const err = e as Error;
if (err instanceof OutdatedExtensionError) {
this.showUpdateExtensionErrorMessage(err);
} else {
} else if (!isTesting()) {
vscode.window.showErrorMessage(err.message);
}
console.error(err.message);
Expand Down
145 changes: 102 additions & 43 deletions src/core/inferFullTargets.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
import {
Mark,
Modifier,
PartialListTargetDescriptor,
PartialPrimitiveTargetDescriptor,
PartialRangeTargetDescriptor,
PartialTargetDescriptor,
PositionModifier,
PrimitiveTargetDescriptor,
RangeTargetDescriptor,
TargetDescriptor,
Expand Down Expand Up @@ -94,36 +97,30 @@ function inferPrimitiveTarget(
};
}

const hasPosition = !!target.modifiers?.find(
(modifier) => modifier.type === "position"
);
const ownPositionalModifier = getPositionalModifier(target);
const ownNonPositionalModifiers = getNonPositionalModifiers(target);

// Position without a mark can be something like "take air past end of line"
// We will remove this case when we implement #736
const mark = target.mark ??
(hasPosition ? getPreviousMark(previousTargets) : null) ?? {
(ownPositionalModifier == null
? null
: getPreviousMark(previousTargets)) ?? {
type: "cursor",
};

const previousModifiers = getPreviousModifiers(previousTargets);
const nonPositionalModifiers =
ownNonPositionalModifiers ??
getPreviousNonPositionalModifiers(previousTargets) ??
[];

const modifiers = target.modifiers ?? previousModifiers ?? [];
const positionalModifier =
ownPositionalModifier ?? getPreviousPositionalModifier(previousTargets);

// "bring line to after this" needs to infer line on second target
const modifierTypes = [
...new Set(modifiers.map((modifier) => modifier.type)),
const modifiers = [
...(positionalModifier == null ? [] : [positionalModifier]),
...nonPositionalModifiers,
];
if (
previousModifiers != null &&
modifierTypes.length === 1 &&
modifierTypes[0] === "position"
) {
const containingScopeModifier = previousModifiers.find(
(modifier) => modifier.type === "containingScope"
);
if (containingScopeModifier != null) {
modifiers.push(containingScopeModifier);
}
}

return {
type: target.type,
Expand All @@ -132,45 +129,107 @@ function inferPrimitiveTarget(
};
}

function getPreviousMark(previousTargets: PartialTargetDescriptor[]) {
return getPreviousTarget(
previousTargets,
(target: PartialPrimitiveTargetDescriptor) => target.mark != null
)?.mark;
function getPositionalModifier(
target: PartialPrimitiveTargetDescriptor
): PositionModifier | undefined {
if (target.modifiers == null) {
return undefined;
}

const positionModifierIndex = target.modifiers.findIndex(
(modifier) => modifier.type === "position"
);

if (positionModifierIndex > 0) {
throw Error("Position modifiers must be at the start of a modifier chain");
}

return positionModifierIndex === -1
? undefined
: (target.modifiers[positionModifierIndex] as PositionModifier);
}

function getPreviousModifiers(previousTargets: PartialTargetDescriptor[]) {
return getPreviousTarget(
/**
* Return a list of non-positional modifiers on the given target. We return
* undefined if there are none. Note that we will never return an empty list; we
* will always return `undefined` if there are no non-positional modifiers.
* @param target The target from which to get the non-positional modifiers
* @returns A list of non-positional modifiers or `undefined` if there are none
*/
function getNonPositionalModifiers(
target: PartialPrimitiveTargetDescriptor
): Modifier[] | undefined {
const nonPositionalModifiers = target.modifiers?.filter(
(modifier) => modifier.type !== "position"
);
return nonPositionalModifiers == null || nonPositionalModifiers.length === 0
? undefined
: nonPositionalModifiers;
}

function getPreviousMark(
previousTargets: PartialTargetDescriptor[]
): Mark | undefined {
return getPreviousTargetAttribute(
previousTargets,
(target: PartialPrimitiveTargetDescriptor) => target.modifiers != null
)?.modifiers;
(target: PartialPrimitiveTargetDescriptor) => target.mark
);
}

function getPreviousTarget(
function getPreviousNonPositionalModifiers(
previousTargets: PartialTargetDescriptor[]
): Modifier[] | undefined {
return getPreviousTargetAttribute(previousTargets, getNonPositionalModifiers);
}

function getPreviousPositionalModifier(
previousTargets: PartialTargetDescriptor[]
): PositionModifier | undefined {
return getPreviousTargetAttribute(previousTargets, getPositionalModifier);
}

/**
* Walks backward through the given targets and their descendants trying to find
* the first target for which the given attribute extractor returns a
* non-nullish value. Returns `undefined` if none could be found
* @param previousTargets The targets that precede the target we are trying to
* infer. We look in these targets and their descendants for the given attribute
* @param getAttribute The function used to extract the attribute from a
* primitive target
* @returns The extracted attribute or undefined if one could not be found
*/
function getPreviousTargetAttribute<T>(
previousTargets: PartialTargetDescriptor[],
useTarget: (target: PartialPrimitiveTargetDescriptor) => boolean
): PartialPrimitiveTargetDescriptor | null {
getAttribute: (target: PartialPrimitiveTargetDescriptor) => T | undefined
): T | undefined {
// Search from back(last) to front(first)
for (let i = previousTargets.length - 1; i > -1; --i) {
const target = previousTargets[i];
switch (target.type) {
case "primitive":
if (useTarget(target)) {
return target;
case "primitive": {
const attributeValue = getAttribute(target);
if (attributeValue != null) {
return attributeValue;
}
break;
case "range":
if (useTarget(target.anchor)) {
return target.anchor;
}
case "range": {
const attributeValue = getAttribute(target.anchor);
if (attributeValue != null) {
return attributeValue;
}
break;
}
case "list":
const result = getPreviousTarget(target.elements, useTarget);
if (result != null) {
return result;
const attributeValue = getPreviousTargetAttribute(
target.elements,
getAttribute
);
if (attributeValue != null) {
return attributeValue;
}
break;
}
}
return null;
return undefined;
}
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
languageId: plaintext
command:
version: 1
spokenForm: bring point and harp to end of second car whale and end of whale take whale
action: replaceWithTarget
spokenForm: bring point and harp to end of second car whale and end of just whale take whale
version: 2
targets:
- type: list
elements:
Expand All @@ -13,15 +12,22 @@ command:
- type: list
elements:
- type: primitive
position: after
insideOutsideType: inside
selectionType: token
modifier: {type: subpiece, pieceType: character, anchor: 1, active: 1, excludeAnchor: false, excludeActive: false}
mark: {type: decoratedSymbol, symbolColor: default, character: w}
modifiers:
- {type: position, position: end}
- type: ordinalRange
scopeType: {type: character}
anchor: 1
active: 1
excludeAnchor: false
excludeActive: false
- type: primitive
position: after
insideOutsideType: inside
mark: {type: decoratedSymbol, symbolColor: default, character: w}
modifiers:
- {type: position, position: end}
- {type: toRawSelection}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that we add "just" here to break inference chain, because we actually had a bug where we weren't doing inference for subtoken modifiers that had a position, even though we should have been

usePrePhraseSnapshot: false
action: {name: replaceWithTarget}
marksToCheck: [default.w]
initialState:
documentContents: hello. world
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
languageId: plaintext
command:
version: 1
spokenForm: >-
bring point and harp to start of second car whale and start of whale take
bring point and harp to start of second car whale and start of just whale take
whale
action: replaceWithTarget
version: 2
targets:
- type: list
elements:
Expand All @@ -15,15 +14,22 @@ command:
- type: list
elements:
- type: primitive
position: before
insideOutsideType: inside
selectionType: token
modifier: {type: subpiece, pieceType: character, anchor: 1, active: 1, excludeAnchor: false, excludeActive: false}
mark: {type: decoratedSymbol, symbolColor: default, character: w}
modifiers:
- {type: position, position: start}
- type: ordinalRange
scopeType: {type: character}
anchor: 1
active: 1
excludeAnchor: false
excludeActive: false
- type: primitive
position: before
insideOutsideType: inside
mark: {type: decoratedSymbol, symbolColor: default, character: w}
modifiers:
- {type: position, position: start}
- {type: toRawSelection}
usePrePhraseSnapshot: false
action: {name: replaceWithTarget}
marksToCheck: [default.w]
initialState:
documentContents: hello. world
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
languageId: plaintext
command:
version: 1
spokenForm: bring point and point to end of second car whale and end of whale take whale
action: replaceWithTarget
spokenForm: bring point and point to end of second car whale and end of just whale take whale
version: 2
targets:
- type: list
elements:
Expand All @@ -13,15 +12,22 @@ command:
- type: list
elements:
- type: primitive
position: after
insideOutsideType: inside
selectionType: token
modifier: {type: subpiece, pieceType: character, anchor: 1, active: 1, excludeAnchor: false, excludeActive: false}
mark: {type: decoratedSymbol, symbolColor: default, character: w}
modifiers:
- {type: position, position: end}
- type: ordinalRange
scopeType: {type: character}
anchor: 1
active: 1
excludeAnchor: false
excludeActive: false
- type: primitive
position: after
insideOutsideType: inside
mark: {type: decoratedSymbol, symbolColor: default, character: w}
modifiers:
- {type: position, position: end}
- {type: toRawSelection}
usePrePhraseSnapshot: false
action: {name: replaceWithTarget}
marksToCheck: [default.w]
initialState:
documentContents: hello. world
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
languageId: plaintext
command:
version: 1
spokenForm: >-
bring point and point to start of second car whale and start of whale take
bring point and point to start of second car whale and start of just whale take
whale
action: replaceWithTarget
version: 2
targets:
- type: list
elements:
Expand All @@ -15,15 +14,22 @@ command:
- type: list
elements:
- type: primitive
position: before
insideOutsideType: inside
selectionType: token
modifier: {type: subpiece, pieceType: character, anchor: 1, active: 1, excludeAnchor: false, excludeActive: false}
mark: {type: decoratedSymbol, symbolColor: default, character: w}
modifiers:
- {type: position, position: start}
- type: ordinalRange
scopeType: {type: character}
anchor: 1
active: 1
excludeAnchor: false
excludeActive: false
- type: primitive
position: before
insideOutsideType: inside
mark: {type: decoratedSymbol, symbolColor: default, character: w}
modifiers:
- {type: position, position: start}
- {type: toRawSelection}
usePrePhraseSnapshot: false
action: {name: replaceWithTarget}
marksToCheck: [default.w]
initialState:
documentContents: hello. world
Expand Down
Loading