TyOverby / ares

A Lisp made for easy integration with Rust.
9 stars 1 forks source link

Tests failing only on nightly branch #32

Open TyOverby opened 9 years ago

TyOverby commented 9 years ago

@bwo

Something in rust changed that breaks our tokenization tests. Could you take a look at it?

bwo commented 9 years ago

hm strange. they work for me. will look.

bwo commented 9 years ago

ah, can reproduce. strange.

bwo commented 9 years ago

On nightly you can successfully parse "+1" as an int, is the reason.

bwo commented 9 years ago

derp actually I'm super tired and the "fix" was to delete the offending test. But really that changes the semantics depending on beta/nightly, which is stupid. I think this ought to be raised as an issue w/ the release, also (but that's independent).

hanna-kruppe commented 9 years ago

I know nothing about this project, I'm one of the people responsible for the change in nightly. Could someone explain the exact cause and extent of this issue to me? From skimming the code, it seems that previously +1 was evaluated as a float and now it's evaluated as integer, because ares::parse::one_expr first tries to parse the string as int and then tries to parse it as float. Is this correct? Is +N really intended to be parsed as float (e.g. do other Lisps do the same?), or did you just go with that because of that's how str::parse used to behave?

And what action would you like upstream to take? The change has landed in 1.5 nightly, it will be in stable releases in about 8 weeks. This is an unfortunate side effect of the trains model, but it's the same for all bug fixes and all other changes. Is this temporary difference between nightly/beta/stable the problem, or is it a problem for you that +1 parses as integer?

Aside: Won't the nearby test parse_ok!("+1.0", 1.0) fail on current stable? If that string is passed to str::parse unaltered, then 1.3 stable will not parse it, but beta and nightly will.

bwo commented 9 years ago

Yes, it seems it will!

I think the simplest thing is just not to allow a leading +. (Rust itself doesn't, which somewhat surprised me.)

hanna-kruppe commented 9 years ago

Alternatively, you could not allow leading + for a few months, until everyone is surely using a release which permits leading + on both ints and floats, and then turn your own support for + back on. That would be the sledgehammer method for avoiding subtle differences between nightly and stable, while still eventually allowing leading +.

TyOverby commented 9 years ago

@rkruppe

This is something that we can certainly fix on our end.

However, waiting for a few months for everyone to be on 1.5-stable is not an option as far as stability goes. I'd really like for this library to work on all rust 1.x compilers; (after all, that was the rust-1.0 promise).

hanna-kruppe commented 9 years ago

I appreciate that, but there is no way to back-port this change into all previously released versions and patch the existing installations. So you are effectively asking for this change to be reverted (if it's to be "fixed" on the Rust side, that is). This is certainly a valid position to take, I just want to make it explicit.

(after all, that was the rust-1.0 promise).

To nitpick: The promise was backwards compatibility, that is, code that works on 1.x continues to work in future releases. Certainly this promise was broken in this case (and in several others; the term used was "hassle-free upgrades" IIRC). However, it's perfectly okay for code to requires, say, Rust 1.5 or later because it depends on a feature or bugfix introduced in 1.5. Whether you consider this change such a feature/bugfix is up to you, I don't blame anyone who doesn't.

TyOverby commented 9 years ago

I think that's a pretty accurate summary. I won't actually request that the patch be reverted because it can be fixed pretty easily on our end (and arguably force us to handle things better than they were in the past).

We are lucky that there are few users of rust and no legacy code.

P.S.

Before I knew what the breaking change is, I checked bitrust and it was entirely useless. Most of the titles for the breaking changes are things like " Auto merge of #25387 - eddyb:syn-file-loader, r=nikomatsakis".

I think we need better infrastructure around educating developers about breaking changes. Even a simple table with

would be miles above what we have now.