Skip to content

Commit b09f5a6

Browse files
committed
refactor: simplify ParseClauseSegment verb-extraction loop
Drop the `quotedFirstVerb` flag and gate the walk solely on `firstVerb is not null` — the QuotedString branch no longer needs a sentinel because it falls through with `firstVerb == null` and the loop short-circuits. Cache the inner `HashSet<string>` from `FlagsWithValue` once into `flagsForVerb` instead of re-hashing `firstVerb` on every flag-token iteration. Inline the `=`-position scan via a single `IndexOf('=')` call so the `--flag=value` short-circuit doesn't traverse the flag string twice; this lets us delete the now-unused `StripEqualsValue` and `HasInlineEqualsValue` helpers. Trim the 20-line block comment at the top of `ParseClauseSegment` to just the load-bearing invariants (FileVerb carveout + ordering with flag-with-value consumption). Drop mid-loop comments that narrate the control flow. Add a 4-element capacity hint to `verbTokens` to avoid the first realloc on the typical case. In `BashVerbs.cs`, revert the `FileVerbs` remarks paragraph that leaked parser usage details and rewrite the `IsVerbLikeToken` doc to capture the WHY of the strict allow-list (vs. negation-of-LooksLikePath) without re-listing all the rejection categories. All 394 tests still pass; no behavior change.
1 parent 6166244 commit b09f5a6

2 files changed

Lines changed: 52 additions & 106 deletions

File tree

src/ShellSyntaxTree/Internal/Bash/Parsing/BashCommandParser.cs

Lines changed: 41 additions & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -769,58 +769,38 @@ private static ClauseResult ParseClauseSegment(
769769
return ClauseResult.Empty();
770770
}
771771

772-
// ---- Verb-chain extraction (Issue #27 greedy heuristic, SPEC §6.1) ----
773-
//
774-
// Walk consecutive verb-like tokens from the start of the clause,
775-
// transparently consuming flag-with-value pairs owned by the first
776-
// verb. Stop at:
777-
// - the first non-Word token (operator, quoted string, opaque, ...)
778-
// - a flag that is not in FlagsWithValue[firstVerb]
779-
// - a Word that does not match IsVerbLikeToken
780-
//
781-
// Two structural carveouts preserve existing per-verb semantics:
782-
// - A QuotedString at index 0 (e.g. `"git" push`) becomes a
783-
// 1-token verb chain; the walk does not continue. Bash semantics
784-
// say the quoted command name is just a verb identity carrier;
785-
// downstream args are arg-list material.
786-
// - When firstVerb is a known FILE verb (cat, ls, bash, cd, chmod,
787-
// grep, find, …) we still run the flag-with-value consumption
788-
// so the value of a `-C /repo` style pair picks up IsPath via
789-
// FlagValueIsPath — but the verb chain stops at the first verb
790-
// token. This keeps per-verb path-arg classification firing for
791-
// bare-name targets (`cat README`, `bash myscript`, `ln src dst`).
772+
// Verb-chain extraction per SPEC §6.1. The FileVerb carveout is
773+
// load-bearing: downstream per-verb positional-arg classification
774+
// depends on the verb chain staying 1 token for FILE verbs so
775+
// bare-name targets like `cat README` still surface as Args with
776+
// IsPath=true. Flag-with-value consumption must run *before* the
777+
// carveout gate so `tar -C /repo` still attributes IsPath to /repo.
792778
var consumedFlagValueIndices = new HashSet<int>();
793-
var verbTokens = new List<string>();
779+
var verbTokens = new List<string>(4);
794780
var verbPositions = new HashSet<int>();
795781

796782
var firstToken = segment.Tokens[0];
797783
string? firstVerb = null;
798-
var quotedFirstVerb = false;
799784
if (firstToken.Kind == BashTokenKind.Word && !IsFlagWord(firstToken))
800785
{
801786
firstVerb = firstToken.Value;
787+
verbTokens.Add(firstVerb);
788+
verbPositions.Add(0);
802789
}
803790
else if (firstToken.Kind == BashTokenKind.QuotedString)
804791
{
805-
firstVerb = firstToken.Value;
806-
quotedFirstVerb = true;
807-
}
808-
809-
if (firstVerb is not null)
810-
{
811-
verbTokens.Add(firstVerb);
792+
// Quoted command (`"git" push`): emit a 1-token chain and skip
793+
// the walk. Bash semantics treat the quoted form as a verb
794+
// identity carrier; remaining tokens are arg-list material.
795+
verbTokens.Add(firstToken.Value);
812796
verbPositions.Add(0);
813797
}
814798

799+
BashVerbs.FlagsWithValue.TryGetValue(firstVerb ?? string.Empty, out var flagsForVerb);
815800
var fileVerbCarveout = firstVerb is not null
816-
&& !quotedFirstVerb
817801
&& BashVerbs.FileVerbs.Contains(firstVerb);
818802

819-
var hasFlagsTable = firstVerb is not null
820-
&& !quotedFirstVerb
821-
&& BashVerbs.FlagsWithValue.TryGetValue(firstVerb, out _);
822-
823-
if (firstVerb is not null && !quotedFirstVerb)
803+
if (firstVerb is not null)
824804
{
825805
for (var i = 1; i < segment.Tokens.Count; i++)
826806
{
@@ -832,42 +812,39 @@ private static ClauseResult ParseClauseSegment(
832812

833813
if (IsFlagWord(t))
834814
{
835-
// Flag-with-value consumption runs whether or not the
836-
// FileVerb carveout is active: tar / curl / wget / git
837-
// all use this path to push the value-token into
838-
// FlagValueIsPath territory.
839-
if (hasFlagsTable
840-
&& BashVerbs.FlagsWithValue[firstVerb].Contains(StripEqualsValue(t.Value))
841-
&& i + 1 < segment.Tokens.Count
842-
&& (segment.Tokens[i + 1].Kind == BashTokenKind.Word
843-
|| segment.Tokens[i + 1].Kind == BashTokenKind.QuotedString))
815+
if (flagsForVerb is null)
844816
{
845-
if (HasInlineEqualsValue(t.Value))
846-
{
847-
// `--flag=value` is a single token; let
848-
// arg-extraction split on `=`. Stop the walk.
849-
break;
850-
}
817+
break;
818+
}
851819

852-
consumedFlagValueIndices.Add(i);
853-
consumedFlagValueIndices.Add(i + 1);
854-
i++; // skip the value too on the next iteration
855-
continue;
820+
var eq = t.Value.IndexOf('=');
821+
var flagKey = eq > 0 ? t.Value.Substring(0, eq) : t.Value;
822+
if (!flagsForVerb.Contains(flagKey))
823+
{
824+
break;
856825
}
857826

858-
// Plain flag (no value consumption) → stops the walk.
859-
break;
860-
}
827+
if (eq > 0)
828+
{
829+
// `--flag=value` is a single token; arg-extraction
830+
// splits on `=`. Stop the walk here.
831+
break;
832+
}
861833

862-
// Non-flag Word past index 0:
863-
if (fileVerbCarveout)
864-
{
865-
// FileVerb 1-token carveout: don't extend the chain,
866-
// but the preceding flag-with-value consumption stays.
867-
break;
834+
if (i + 1 >= segment.Tokens.Count
835+
|| (segment.Tokens[i + 1].Kind != BashTokenKind.Word
836+
&& segment.Tokens[i + 1].Kind != BashTokenKind.QuotedString))
837+
{
838+
break;
839+
}
840+
841+
consumedFlagValueIndices.Add(i);
842+
consumedFlagValueIndices.Add(i + 1);
843+
i++;
844+
continue;
868845
}
869846

870-
if (!BashVerbs.IsVerbLikeToken(t))
847+
if (fileVerbCarveout || !BashVerbs.IsVerbLikeToken(t))
871848
{
872849
break;
873850
}
@@ -958,20 +935,6 @@ private static bool IsFlagWord(BashToken token)
958935
return token.Value.Length > 0 && token.Value[0] == '-';
959936
}
960937

961-
/// <summary>
962-
/// For an equals-form flag like <c>--output=file.txt</c>, return the
963-
/// flag portion (<c>--output</c>) so the FlagsWithValue table lookup
964-
/// matches. For plain flags returns the input unchanged.
965-
/// </summary>
966-
private static string StripEqualsValue(string flag)
967-
{
968-
var eq = flag.IndexOf('=');
969-
return eq > 0 ? flag.Substring(0, eq) : flag;
970-
}
971-
972-
private static bool HasInlineEqualsValue(string flag) =>
973-
flag.IndexOf('=') > 0;
974-
975938
// ---------------------------------------------------------------- args + redirects
976939

977940
/// <summary>

src/ShellSyntaxTree/Internal/Bash/Verbs/BashVerbs.cs

Lines changed: 11 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -43,18 +43,8 @@ internal static class BashVerbs
4343
/// modulo per-verb overrides in SPEC §7. SPEC §6.3.
4444
/// </summary>
4545
/// <remarks>
46-
/// <para>
4746
/// CWD verbs are also FILE verbs (their target is a path), included
4847
/// here for closure so a single membership check suffices.
49-
/// </para>
50-
/// <para>
51-
/// Issue #27: this set ALSO acts as the "stop at 1-token verb chain"
52-
/// carveout in <c>BashCommandParser.ParseClauseSegment</c>. The
53-
/// greedy verb-chain heuristic would otherwise over-extract bare-name
54-
/// targets (<c>cat hello</c>, <c>bash myscript</c>, <c>ln src dst</c>)
55-
/// into the verb chain and lose the per-verb path-arg classification
56-
/// downstream consumers depend on for zone-gate evaluation.
57-
/// </para>
5848
/// </remarks>
5949
internal static readonly HashSet<string> FileVerbs =
6050
new(StringComparer.OrdinalIgnoreCase)
@@ -130,29 +120,22 @@ internal static readonly IReadOnlyDictionary<string, HashSet<string>>
130120
};
131121

132122
/// <summary>
133-
/// Issue #27 / SPEC §6.1: returns <c>true</c> when <paramref name="token"/>
134-
/// has the shape of a CLI subcommand verb — a bare lowercase identifier
123+
/// SPEC §6.1: returns <c>true</c> when <paramref name="token"/> has the
124+
/// shape of a CLI subcommand verb — a bare lowercase identifier
135125
/// containing only ASCII letters, digits, hyphens, dots, and underscores.
136126
/// Used to terminate the greedy verb-chain walk at the first token that
137127
/// looks like a value rather than another subcommand.
138128
/// </summary>
139129
/// <remarks>
140-
/// <para>
141-
/// The shape predicate is intentionally strict: leading ASCII lowercase
142-
/// letter, then only <c>[a-z0-9._-]</c>. This rejects flags (start with
143-
/// <c>-</c>), env-var refs (<c>$</c>), path-shapes (<c>/</c>, <c>\</c>,
144-
/// <c>~</c>), URLs (<c>:</c>), glob metachars (<c>*</c>, <c>?</c>,
145-
/// <c>[</c>), uppercase-starting tokens (user-named identifiers like
146-
/// migration names), and tokens that begin with a digit (numeric
147-
/// modes / version literals). It tolerates real subcommand shapes
148-
/// (<c>my-pod</c>, <c>s3</c>, <c>apt-get</c>, <c>python3.11</c>).
149-
/// </para>
150-
/// <para>
151-
/// Quoted strings are not verb-like even when their inner value would
152-
/// pass — quoting signals the user wanted the bytes as a literal
153-
/// value. The walk also rejects empty tokens and tokens longer than
154-
/// 64 characters as a defensive bound against pathological inputs.
155-
/// </para>
130+
/// Strict allow-list (leading <c>[a-z]</c>, body <c>[a-z0-9._-]</c>)
131+
/// over the more obvious negation-of-LooksLikePath because it stays
132+
/// conservative for unknown shapes: a token like <c>readme.md</c>
133+
/// satisfies the allow-list and would extend an unknown CLI's verb
134+
/// chain, but the FileVerb carveout in <c>BashCommandParser</c>
135+
/// short-circuits the common case (<c>cat readme.md</c>) before the
136+
/// allow-list ever runs. Quoted strings are excluded so the user's
137+
/// intent to treat bytes literally is preserved. The 64-char bound
138+
/// is a defensive cap against pathological inputs.
156139
/// </remarks>
157140
internal static bool IsVerbLikeToken(in BashToken token)
158141
{

0 commit comments

Comments
 (0)