Plumbed spans through alias definitions, allowing for more precise er…

[rben01]

…ror reporting of aliases conflicting with existing names

Simplified implementation of name_and_aliases (and name_and_aliases_spans, which returns spans); now only allocates once, and return a concrete impl Iterator (not Box<dyn Iterator>)

Closes #570

Before:

>>> @name("Malaysian ringgit")
@url("https://en.wikipedia.org/wiki/Malaysian_ringgit")
@aliases(malaysian_ringgits, MYR: short, myr, RM, rm)
unit malaysian_ringgit: Money = EUR / exchange_rate("MYR")
error: identifier clash in definition
   ┌─ Module 'units::time', File /opt/homebrew/Cellar/numbat/1.13.0/share/numbat/modules/units/time.nbt:32:6
   │
32 │ unit year: Time = 365.242_188_1 days
   │      ---- Previously defined here
   │
   ┌─ <input:7>:4:6
   │
 4 │ unit malaysian_ringgit: Money = EUR / exchange_rate("MYR")
   │      ^^^^^^^^^^^^^^^^^ identifier is already in use

After:

>>> @name("Malaysian ringgit")
@url("https://en.wikipedia.org/wiki/Malaysian_ringgit")
@aliases(malaysian_ringgits, MYR: short, myr, RM, rm)
unit malaysian_ringgit: Money = EUR / exchange_rate("MYR")
error: identifier clash in definition
   ┌─ Module 'units::time', File <builtin>/modules/units/time.nbt:32:6
   │
32 │ unit year: Time = 365.242_188_1 days
   │      ---- Previously defined here
   │
   ┌─ <input:1>:3:42
   │
 3 │ @aliases(malaysian_ringgits, MYR: short, myr, RM, rm)
   │                                          ^^^ identifier is already in use

I debated whether to point the “previously defined here” error span not to year itself but to the alias responsible, yr, but thought that that was less helpful, as for more obscure aliases it won't be obvious what unit it is tied to (without using info <alias>). Also the line containing the alias often has lots of extraneous info such as other aliases or alias config. I think it's nicer to have the full chain of conflict laid out all the way back to the source, the original unit, which in theory should be quite obvious — everyone knows what unit year: Time = 365.242_188_1 days means.

[rben01]

I need help writing tests for this.

[rben01]

For future work it would be interesting to report all conflicting identifiers in one go. For instance, rm is also an existing unit (rontometre), but the error reporting bails after the first issue. This would require NameResolutionError to store a Vec or to be able to provide a Vec<NameResolutionError>.

[sharkdp]

I need help writing tests for this.

I'm not sure we have tests that check against the full diagnostic output. It would be a good idea to have this though. Tests in numbat/tests/interpreter.rs probably come closes. Using insta-based snapshot tests for this would probably be a good idea.

If numbat/tests/interpreter.rs doesn't work (no access to diagnostics), we can also put those kind of tests in numbat-cli/tests/intergration.rs. There, we should definitely have access to the full error output.

For future work it would be interesting to report all conflicting identifiers in one go.

I think it's okay to go with just one for now. I don't think it's very common that you hit multiple in one go :-)

[sharkdp]

I can help with creating tests for this (will take a few days though).

[sharkdp]

Sorry. I was interrupted while working on this. Will try to fix the tests on all platforms later today.

[sharkdp]

I reverted my changes. I tried to use insta-cmd to compare the output of the numbat program using insta. But I didn't manage to make it work cross-platform. And I somehow like the existing assert_cmd tests more. And the insta-cmd based tests required additional cargo build steps in CI / during local development. I'll merge this without a regression test for now.

Thank you very much for this.