Protocol: debug_assert!() Span to reflect a valid slice

[dbuch]

Also enforce this by #[non_exhaustive] span such that going forward we cannot, in debug builds (1), construct invalid spans.

The motivation for this stems from #6431 where I've seen crashes due to invalid slice indexing.

My hope is this will mitigate such senarios

1. https://github.com/nushell/nushell/pull/6431#issuecomment-1278147241

Description

(description of your pull request here)

Tests

Make sure you've done the following:

• [ ] Add tests that cover your changes, either in the command examples, the crate/tests folder, or in the /tests folder.
• [ ] Try to think about corner cases and various ways how your changes could break. Cover them with tests.
• [ ] If adding tests is not possible, please document in the PR body a minimal example with steps on how to reproduce so one can verify your change works.

Make sure you've run and fixed any issues with these commands:

• [ ] cargo fmt --all -- --check to check standard code formatting (cargo fmt --all applies these changes)
• [ ] cargo clippy --workspace --features=extra -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect to check that you're using the standard code style
• [ ] cargo test --workspace --features=extra to check that all the tests pass

Documentation

• [ ] If your PR touches a user-facing nushell feature then make sure that there is an entry in the documentation (https://github.com/nushell/nushell.github.io) for the feature, and update it if necessary.

[sholderbach]

That sounds like an interesting way to catch bugs here. The first look at the test failures looks like we have some issues that get caught that way.

[dbuch]

That sounds like an interesting way to catch bugs here. The first look at the test failures looks like we have some issues that get caught that way.

Yeah that's my impression too. Currently I hit a wall because I know little to nothing about the virtualenv. It would be cool if I could figure out how to spin this up locally.

Alternativly we could enable RUST_BACKTRACE=1 in CI? Potentially it could be usefull - or spammy :) Perhaps both.

Below theres a snippet from the CI out put. I think we have a culprint here w.r.t. alias problems. invoke = ['/home/runner/.cargo/bin/nu', '/tmp/pytest-of-runner/pytest-0/test_nushell_no_prompt_0/script.nu'] line = b"alias deactivate = source '/tmp/pytest-of-runner/pytest-0/activation-tester-env1/e-$ \xc3\xa8\xd1\x80\xd1\x82\xf0\x9f\x9a\x92\xe2\x99\x9e\xe4\xb8\xad\xe7\x89\x87-j/bin/deactivate.nu'" monkeypatch = <_pytest.monkeypatch.MonkeyPatch object at 0x7f1ab4e17b80> out = ["thread 'main' panicked at 'Can't create a Span whose end < start, start=6540, end=3876', crates/nu-protocol/src/span.rs:32:9", 'note: run withRUST_BACKTRACE=1 environment variable to display a backtrace']

[sholderbach]

I think @kubouch is the expert on the Python virtualenv integration. He may be able to help you out there.

[kubouch]

You need to install Python and virtualenv, then create a new virtual environment, then try to source the generated activate.nu and/or deactivate.nu. I'm quite busy nowadays to look into it but you should be able to look into virtualenv docs how to do it, it's easy, it's just a few commands.

The template activation scripts are here https://github.com/pypa/virtualenv/tree/main/src/virtualenv/activation/nushell but you can't source them directly, they need to be generated by virtualenv to replace the __VIRTUAL_ENV__ etc. with actual values.

[fdncred]

@dbuch What's the status on this PR?

[fdncred]

Last we talked about this one, I thought we decided it was good to land. We just need to get the ci green. Please correct me if I'm wrong.

[dbuch]

Last we talked about this one, I thought we decided it was good to land. We just need to get the ci green. Please correct me if I'm wrong.

Sorry, i've been quite busy lately. Yes I's my impression we want this. Problem is i did manage to spin up Virtual Python environment, as @kubouch said it's easy :) At it's core atleast. I sadly couldn't trigger the problem seen in CI.

I'll rebase asap and se what happens.

[dbuch]

I make some progress today and caught a Backtrace locally. With some local changes to virtualenv.

Next step will be to look for some additional context around the panic around nu_parser::parser::parse_external_call

[sholderbach]

Yippie! Everything going green! Great work @dbuch. I think we need a few reviewer looks if the fixes are straightforward or hiding a larger problem but then we are good to land this one.

[dbuch]

Yippie! Everything going green! Great work @dbuch. I think we need a few reviewer looks if the fixes are straightforward or hiding a larger problem but then we are good to land this one.

I'm sure this should recieve some more love. Thats why i didn't my self mark as ready for review - on the other hand i don't mind do it in plenum. So if anyone has something to say or want to poke at this "fix" - please go ahead :)

I'll try to elaborate abit, but i need to collect some more data. I just ran out of time and frankly I won't be able to allocate much focus on matter this until the next weekend, i think.

[fdncred]

Hey @dbuch, If we can fix these conflicts we think we'll be able to land this. Let us know if you have time and can work on it. Thanks!

[kubouch]

Thanks!