Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

restrict numeric promotions to cases where no data is lost #4971

Merged
merged 41 commits into from
May 27, 2020

Conversation

UweKrueger
Copy link
Member

@UweKrueger UweKrueger commented May 21, 2020

Problem

Numeric calculations involving different types give unexpected results. Example:

12.3 + 5 -> 17.3
5 + 12.3 -> 17
-100000 + u16(30000) -> -70000
u16(30000) - 100000   -> 61072

Reason

v assumes the type of an expression to be the type of the left hand side, whereas the underlying C performs numeric promotions. These two concept do not fit very well together.

Solution (updated)

This PR restricts numeric promotions to those where the target type is as strict superset of the original type as described in the scheme below. In brief: when there is no risk of data loss.

To handle integer and float literals two new internal types any_int and any_float are introduced that can convert to any type of their class, so things like a := 3 + byte(b) or x := 3.2 * (f32(y) + 4) work in the way that a is a byte and x is a f32.

@UweKrueger UweKrueger marked this pull request as ready for review May 21, 2020 09:34
@spytheman
Copy link
Member

spytheman commented May 21, 2020

I personally prefer the way V works now. It is simpler and more predictable, since you just have to remember one rule to understand what the result would be, and what its type would be.
@medvednikov what do you think?

@UweKrueger
Copy link
Member Author

UweKrueger commented May 21, 2020

@spytheman : I understand your argument but there are really wired examples like:

2 * 6.02214076e23 -> 2147483647

or even:

1.1 * (1 + 0.5) -> 1.65 // seems ok

x := 1 + 0.5
1.1 * x -> 1.1          // seems strange

I don't think that this behavior is accepted/understood by many users. Think of someone unbiased just having heard of v and playing with the REPL or the playground.

@UweKrueger
Copy link
Member Author

UweKrueger commented May 21, 2020

There are of cause other possibilities to address this:

  • require that both operands are of the same type. Go follows this approach - but they have a concept of untyped numeric constants that handles cases like 2*a
  • another approach would be to do strong type promotions that never change a value or lose precision, so no conversion from signed to unsigned or int64 to double. This is something I've thought about but this would break many expressions in existing code.
  • if we stick to the simple rule "always take the type from the left operand" we should at least enforce it for intermediate results by an explicit type conversion in cgen so that both v and C have the same idea of the type. 1.1 * (1 + 0.5) would compute to 1.1 then, too. However, I would not really like this - not for primitive types.

@spytheman
Copy link
Member

I know, and understand that most languages do automatic promotions. It is just a personal preference, and if most people expect it/want it, I guess it is good for V to provide it.

@medvednikov
Copy link
Member

Yeah V is like Go, no magic casting rules, everything must be explicit.

I'm considering only allowing automatic casting from a smaller to a larger type like handle_64(f32)

@UweKrueger
Copy link
Member Author

@medvednikov Actually, I like the way Go handles this, too. But this would mean that a lot of existing v code had to be rewritten since all cases of type mismatch would trigger error messages.

What about this scheme:

   i8 → i16 → int → i64
            ↘     ↘
              f32 → f64
            ↗     ↗
 byte → u16 → u32 → u64 ⬎
      ↘     ↘     ↘      ptr
   i8 → i16 → int → i64 ⬏

There is no data loss but things like 2 * 3.4 would work. (This is no issue in Go because of arbitrary precision untyped numeric constants).

@medvednikov
Copy link
Member

medvednikov commented May 21, 2020

Yeah this is fine, numeric consts are also going to be untyped.

@UweKrueger UweKrueger changed the title numeric promotions for primitive types - as in C numeric promotions for primitive types when no data loss May 22, 2020
@UweKrueger UweKrueger marked this pull request as draft May 22, 2020 05:49
vlib/v/gen/cgen.v Outdated Show resolved Hide resolved
@UweKrueger
Copy link
Member Author

I've basically implemented the above type scheme where only lossless automatic promotions are allowed. Also there are now two "untyped" types any_int and any_float that are used as intermediate characterization of number literals.
Since this is a huge change in the type system, many problems show up in the test cases. Some are just changed wordings in error messages a few are remaining bugs in the compiler's type system but a large amount are real errors of the type "u32 -> int not allowed".
It's clear that there is a lot of work remaining before this PR can be merged. @medvednikov, @spytheman: Before I continue, could you please have a look at the error messages of the tests to evaluate the impact of the stricter type checks? I'd like to hear your opinion, if this is the way to go.

@UweKrueger UweKrueger requested a review from spytheman May 25, 2020 12:26
@@ -50,7 +50,7 @@ pub fn new_test_session(_vargs string) TestSession {
skip_files: skip_files
vargs: vargs
show_ok_tests: !_vargs.contains('-silent')
message_handler: 0
message_handler: &TestMessageHandler{}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be &TestMessageHandler(0) to avoid an extra allocation.

Maybe we should use &TestMessageHandler(none) instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've used &TestMessageHandler(0) for now. But I agree that optionals would be better alternatives in such cases.

run: |
git clone --depth 1 https://github.com/vlang/vid.git
cd vid && ../v -o vid .
# - name: Test vid
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be brought back now

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Back in...

@medvednikov
Copy link
Member

Great job, looks good.

@UweKrueger
Copy link
Member Author

I think, that can be merged now. There is potential for some more fine tuning and merging of checks that can be addressed in future PRs.

@medvednikov medvednikov merged commit 013fdb8 into vlang:master May 27, 2020
@spytheman
Copy link
Member

Great job @UweKrueger !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants