Editorial: Increase consistency and clarity of string operations

[gibson042]

There was a lot of gratuitous divergence between Encode, Decode, escape, and unescape, and also between those operations and other string operations.

Non-comprehensive improvements:

• Make better use of ParseText to get Parse Nodes or errors rather than testing syntax first.
• Align on code {unit,point} whose numeric value is… phrasing rather than code {unit,point} whose value is….
• Increase the majority of Let _len_ be the length of… over alternate aliases for _len_.
• Increase the majority of Repeat, while _k_ < _len_ iteration over alternatives like while _k_ ≠ _len_ or embedded If _k_ = _len_, return….
• Make better use of string-concatenation accepting both strings and code units.

[jmdyck]

These re-use the aliases _hexDigits_, _parseResult_, and _B_, so should use Set-to syntax rather than Let-be.

[gibson042]

I consider these to be separate aliases in a deeper scope that shadow those in the outer scope, which has the joint benefits of a) matching likely implementation where shadowing is available, and b) allowing reuse of the pattern without gratuitous variation.

[jmdyck]

I consider these to be separate aliases in a deeper scope that shadow those in the outer scope,

That's reasonable from a programming language point of view, but I don't think there's anything in the spec that supports it. E.g., 5.2 "Algorithm Conventions" doesn't say anything about scopes or shadowing. It could, but my guess is, that would be more bother than it's worth.

Anyway, I'll leave it to the editors.

[bakkot]

I agree with @jmdyck; we don't currently define any scoping rules except for Abstract Closures, and it's not worth the complexity of doing so vs just picking different aliases or (my preference here) using Set.

Also, nested scoping would conflict with the current pattern of

1. If condition,
  1. Let _x_ be 1.
1. Else,
  1. Let _x_ be 2.
1. Use _x_.

[gibson042]

Sorry, I meant "deeper scope" from a logical perspective rather than algorithmic. AFAICT from Algorithm Conventions, Let _x_ be … after a preceding Let _x_ be … is not an error and has the effect of declaring a new alias that shadows the previous one temporally rather than lexically (i.e., "may be referenced in any subsequent steps"), which in the case of this algorithm are indistinguishable anyway because these declarations are in the same loop body (itself also supporting the "temporal shadowing" interpretation because each iteration encounters the same Let steps) and there are no references at decreasing indentation.

As noted above, I think that switching to Set would result in gratuitous variation for identical logic:

               1. Let _hexDigits_ be the substring of _string_ from _k_ + 1 to _k_ + 3.
               1. Let _parseResult_ be ParseText(StringToCodePoints(_hexDigits_), |HexDigits[~Sep]|).
               1. If _parseResult_ is not a Parse Node, throw a *URIError* exception.
               1. Let _B_ be the unsigned 8-bit value corresponding with the MV of _parseResult_.
               1. …
               1. If _n_ = 0, then
                 1. …
               1. Else,
                 1. …
                 1. Repeat, while _j_ < _n_,
                   1. …
-                  1. Let _hexDigits_ be the substring of _string_ from _k_ + 1 to _k_ + 3.
-                  1. Let _parseResult_ be ParseText(StringToCodePoints(_hexDigits_), |HexDigits[~Sep]|).
+                  1. Set _hexDigits_ to the substring of _string_ from _k_ + 1 to _k_ + 3.
+                  1. Set _parseResult_ to ParseText(StringToCodePoints(_hexDigits_), |HexDigits[~Sep]|).
                   1. If _parseResult_ is not a Parse Node, throw a *URIError* exception.
-                  1. Let _B_ be the unsigned 8-bit value corresponding with the MV of _parseResult_.
+                  1. Set _B_ to the unsigned 8-bit value corresponding with the MV of _parseResult_.

If that is indeed the editorial preference, I think I would instead encapsulate it in a micro-operation, e.g.:

-              1. Let _hexDigits_ be the substring of _string_ from _k_ + 1 to _k_ + 3.
-              1. Let _parseResult_ be ParseText(StringToCodePoints(_hexDigits_), |HexDigits[~Sep]|).
-              1. If _parseResult_ is not a Parse Node, throw a *URIError* exception.
-              1. Let _B_ be the unsigned 8-bit value corresponding with the MV of _parseResult_.
+              1. Let _B_ be ReadHexOctet(_string_, _k_ + 1).
+              1. If _B_ is not an integer, throw a *URIError* exception.
               1. …
               1. If _n_ = 0, then
                 1. …
               1. Else,
                 1. …
                 1. Repeat, while _j_ < _n_,
                   1. …
-                  1. Let _hexDigits_ be the substring of _string_ from _k_ + 1 to _k_ + 3.
-                  1. Let _parseResult_ be ParseText(StringToCodePoints(_hexDigits_), |HexDigits[~Sep]|).
-                  1. If _parseResult_ is not a Parse Node, throw a *URIError* exception.
-                  1. Let _B_ be the unsigned 8-bit value corresponding with the MV of _parseResult_.
+                  1. Let _continuationByte_ be ReadHexOctet(_string_, _k_ + 1).
+                  1. If _continuationByte_ is not an integer, throw a *URIError* exception.

[bakkot]

From the call today, editors are in agreement that we don't want to do this kind of shadowing. If you want to introduce a new AO here that's fine.

[gibson042]

Done.

[bakkot]

LGTM other than nits.

[gibson042]

Thanks @jmdyck; good suggestions.

[jmdyck]

Looks okay to me.

#2974 (review)

[syg]

lgtm % small comments