Clean unsupported chars from implicit namespace name

[swernli]

This change adds some simple logic to clean up unsupported characters from file names before they are used as implicit namespace names. It is by no means exhaustive, but at least takes any - replaces them with _. This will applied for all files, including unsaved ones, so a file saved to disk as MyCool-File.qs would become the namespace MyCool_File and VS Code's somewhat quirky names for new unsaved file buffers like untitled:Untitled-1 becomes Untitled_1. This allows skipping explicit namespaces in unsaved new files, fixes #1582.

[github-actions[bot]]

Benchmark for 8c2ee9c

Click to view benchmark

| Test | Base | PR | % | |------|--------------|------------------|---| | Array append evaluation | 330.6±2.22µs | 328.8±8.65µs | -0.54% | | Array literal evaluation | 192.7±1.78µs | 194.2±3.15µs | +0.78% | | Array update evaluation | 407.0±6.06µs | 407.7±9.04µs | +0.17% | | Core + Standard library compilation | **20.9±0.64ms** | 22.9±0.91ms | **+9.57%** | | Deutsch-Jozsa evaluation | 5.1±0.05ms | 5.1±0.05ms | 0.00% | | Large file parity evaluation | **33.9±0.10ms** | 34.1±0.39ms | **+0.59%** | | Large input file compilation | **13.0±0.34ms** | 13.8±0.34ms | **+6.15%** | | Large input file compilation (interpreter) | **50.8±1.13ms** | 55.0±1.33ms | **+8.27%** | | Large nested iteration | 32.8±2.48ms | 32.3±0.77ms | -1.52% | | Perform Runtime Capabilities Analysis (RCA) on Deutsch-Jozsa sample | 1561.5±48.55µs | 1578.7±125.02µs | +1.10% | | Perform Runtime Capabilities Analysis (RCA) on large file sample | 8.0±0.13ms | 7.9±0.14ms | -1.25% | | Perform Runtime Capabilities Analysis (RCA) on teleport sample | 1421.8±49.99µs | 1442.7±74.61µs | +1.47% | | Perform Runtime Capabilities Analysis (RCA) on the core and std libraries | 28.3±1.15ms | 28.6±0.42ms | +1.06% | | Teleport evaluation | 87.4±3.62µs | 88.8±4.81µs | +1.60% |

[github-actions[bot]]

Benchmark for 4496267

Click to view benchmark

| Test | Base | PR | % | |------|--------------|------------------|---| | Array append evaluation | **325.5±2.25µs** | 328.7±1.34µs | **+0.98%** | | Array literal evaluation | 202.4±4.52µs | **193.8±2.55µs** | **-4.25%** | | Array update evaluation | 409.7±2.63µs | **405.6±1.52µs** | **-1.00%** | | Core + Standard library compilation | 21.6±1.19ms | **20.9±0.57ms** | **-3.24%** | | Deutsch-Jozsa evaluation | 5.1±0.05ms | **5.0±0.05ms** | **-1.96%** | | Large file parity evaluation | **33.9±0.09ms** | 34.0±0.22ms | **+0.29%** | | Large input file compilation | 13.0±0.37ms | 13.2±0.67ms | +1.54% | | Large input file compilation (interpreter) | 51.6±1.63ms | 51.0±1.23ms | -1.16% | | Large nested iteration | **32.0±0.15ms** | 32.2±0.14ms | **+0.62%** | | Perform Runtime Capabilities Analysis (RCA) on Deutsch-Jozsa sample | 1573.6±65.66µs | 1579.8±53.66µs | +0.39% | | Perform Runtime Capabilities Analysis (RCA) on large file sample | 8.0±0.15ms | 7.9±0.10ms | -1.25% | | Perform Runtime Capabilities Analysis (RCA) on teleport sample | 1429.2±38.29µs | 1432.5±40.74µs | +0.23% | | Perform Runtime Capabilities Analysis (RCA) on the core and std libraries | 28.3±0.25ms | 28.3±0.36ms | 0.00% | | Teleport evaluation | 87.9±4.01µs | 88.6±9.85µs | +0.80% |

[minestarks]

@swernli In untitled:Untitled-1, the : isn't part of the file name, it's the URL scheme separator (same as the colon in http://foo). To handle that, I think a better fix would be to update truncate_to_path_separator so it truncates back to the : if it can't find a slash. (Bonus: This would handle the technically valid C:foo.qs Windows path too, though I don't know what kind of crazy use case would land you in that scenario).

I think - -> _ is a good conversion. I can see how that's necessary for the Untitled file case.

But otherwise, can we keep it very conservative? I'm not as sold on converting spaces. If we open up the door to more filename collisions, it can get confusing IMO. I don't feel too strongly about this last point though.

[swernli]

@swernli In untitled:Untitled-1, the : isn't part of the file name, it's the URL scheme separator (same as the colon in http://foo). To handle that, I think a better fix would be to update truncate_to_path_separator so it truncates back to the : if it can't find a slash. (Bonus: This would handle the technically valid C:foo.qs Windows path too, though I don't know what kind of crazy use case would land you in that scenario).

I think - -> _ is a good conversion. I can see how that's necessary for the Untitled file case.

But otherwise, can we keep it very conservative? I'm not as sold on converting spaces. If we open up the door to more filename collisions, it can get confusing IMO. I don't feel too strongly about this last point though.

I can make the change to truncate_to_path_separator, thanks for pointing to that! Should I also drop the handling for ` (space) turning into-`?

[github-actions[bot]]

Benchmark for fbfd65e

Click to view benchmark

| Test | Base | PR | % | |------|--------------|------------------|---| | Array append evaluation | 328.2±2.99µs | 330.4±5.27µs | +0.67% | | Array literal evaluation | 175.0±1.09µs | 175.1±1.80µs | +0.06% | | Array update evaluation | 406.9±3.08µs | 409.8±9.89µs | +0.71% | | Core + Standard library compilation | 21.0±0.86ms | 20.9±0.60ms | -0.48% | | Deutsch-Jozsa evaluation | **5.1±0.06ms** | 5.2±0.22ms | **+1.96%** | | Large file parity evaluation | 34.2±0.40ms | 34.3±1.00ms | +0.29% | | Large input file compilation | 12.6±0.25ms | **12.4±0.11ms** | **-1.59%** | | Large input file compilation (interpreter) | 51.0±1.58ms | **48.2±0.85ms** | **-5.49%** | | Large nested iteration | 32.6±0.33ms | 32.9±0.52ms | +0.92% | | Perform Runtime Capabilities Analysis (RCA) on Deutsch-Jozsa sample | **1571.5±44.35µs** | 1611.0±35.78µs | **+2.51%** | | Perform Runtime Capabilities Analysis (RCA) on large file sample | 7.9±0.10ms | 7.9±0.20ms | 0.00% | | Perform Runtime Capabilities Analysis (RCA) on teleport sample | 1434.3±84.98µs | 1454.2±56.47µs | +1.39% | | Perform Runtime Capabilities Analysis (RCA) on the core and std libraries | 28.3±0.21ms | **27.9±0.23ms** | **-1.41%** | | Teleport evaluation | 89.2±3.35µs | 91.7±6.55µs | +2.80% |

[sezna]

I can make the change to truncate_to_path_separator, thanks for pointing to that! Should I also drop the handling for ` (space) turning into-`?

I like removing the handling for `, since it isn't an issue in your original scenario -- we should avoid being too liberal with this conversion, and specifically handling your scenario (theuntitled:Untitled-1` situation) is best IMO.

[swernli]

I've updated the logic to only convert - -> _ which solves the main case I was concerned with. We'll avoid any other special handling until it's necessary.