fix: f32 and f64 representation during lowering #12395

feniljain · 2022-05-26T11:38:03Z

should fix #12380

lnicola · 2022-05-26T12:07:48Z

crates/syntax/src/ast/token_ext.rs

+
+#[derive(Default, Debug, Clone, Eq, PartialEq)]
+pub struct FloatTypeWrapper {
+    //We convert float values into bits and that's how we don't need to deal with f32 and f64.


Nit: missing space after // and I'd probably go for a tuple struct (struct FloatEq(u64);), but it doesn't matter too much.

Using tuple struct seemed good, fixed the nit too!

Also sorry for bringing it up here, tho I am not able to reach Jake ( @~jhgg ) for further review on this PR:
#12347

I try to press the re-request review button, but it doesn't do anything :(

Can you have a look at that PR too? 😅

Veykril · 2022-05-26T13:13:19Z

crates/syntax/src/ast/token_ext.rs

+    pub fn float_value(&self) -> Option<FloatTypeWrapper> {
+        let (_, text, _) = self.split_into_parts();
+        let float_value = text.parse::<f64>().ok()?;
+        Some(FloatTypeWrapper::new(float_value))
+    }
+}
+
+// We convert float values into bits and that's how we don't need to deal with f32 and f64.
+// For PartialEq, bits comparison should work, as ordering is not important
+// https://github.com/rust-lang/rust-analyzer/issues/12380#issuecomment-1137284360
+#[derive(Default, Debug, Clone, Eq, PartialEq)]
+pub struct FloatTypeWrapper(u64);
+
+impl FloatTypeWrapper {
+    fn new(value: f64) -> Self {
+        Self(value.to_bits())
+    }
+}
+
+impl std::fmt::Display for FloatTypeWrapper {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "{}", f64::from_bits(self.0))
+    }


This shouldn't go into syntax, let's put this where it is used by hir-def

Shifted it to same file where Literal::Float is defined

Veykril · 2022-05-26T13:13:37Z

crates/syntax/src/ast/token_ext.rs

+    pub fn value(&self) -> Option<FloatTypeWrapper> {
+        let float_value = self.text().parse::<f64>().ok()?;
+        Some(FloatTypeWrapper::new(float_value))
+    }


Similarly this should just return the f64

Made changes for the same

Veykril · 2022-05-26T13:14:55Z

crates/syntax/src/ast/token_ext.rs

@@ -331,6 +355,11 @@ impl ast::FloatNumber {
        }
        Some(&text[suffix_start..])
    }
+
+    pub fn value(&self) -> Option<FloatTypeWrapper> {
+        let float_value = self.text().parse::<f64>().ok()?;


This will fail when we have things like 1.0f32, we should probably have a split_into_parts akin to ast::IntNumber (minus the prefix part).

Yeah, that's right, implemented the same too!

HKalbasi · 2022-05-26T14:27:39Z

crates/ide/src/hover/tests.rs

+        expect![[r#"
+            *FOO*
+
+            ```rust


It might be good to move these tests to hir_ty::consteval::tests

So just have one float literal test here and move other cases to consteval?

Edit: Actually should they go in consteval this was a general problem regarding handling of floats, and they are being used in other places too

So just have one float literal test here and move other cases to consteval?

Yes, that sounds good to me.

Actually should they go in consteval this was a general problem regarding handling of floats

I think it only affects consteval, and current tests just test this, but if there are other problems fixed/affected by this, then adding test for those in addition to consteval tests is good.

feniljain · 2022-05-27T06:45:09Z

So, while re-reviewing the PR and understanding more from the review of @HKalbasi , I realized this does not fix consteval for floats completely, it does fix the base case of a simple float value directly assigned. Exactly what current tests show.

But for cases like const FOO: f64 = 2.0 + 3.0, this won't work because that part is errored out currently, you can see that here:

rust-analyzer/crates/hir-ty/src/consteval.rs

Lines 222 to 226 in 732eb9a

    
           ComputedExpr::Literal(Literal::Int(v, _)) => v, 
        
           ComputedExpr::Literal(Literal::Uint(v, _)) => { 
        
               v.try_into().map_err(|_| ConstEvalError::NotSupported("too big u128"))? 
        
           } 
        
           _ => return Err(ConstEvalError::NotSupported("this kind of operator")),

Though it does fix exactly what the title mentions, so to close the issue #12380 completely I would have to add support for float in consteval.rs too. Now that seems to be some work, testing support for it has to be added, then there would be some significant changes in consteval.rs.

I was wondering should I do it in the same PR or separate one?

We can merge the current one for basic support and as a partial fix to #12380 , then the remaning fix can be made in a separate PR

cc: @lnicola @Veykril @HKalbasi

HKalbasi · 2022-05-27T07:02:04Z

I think it is fine to not support float operations for now, since hover on const FOO: f64 = 2.0 + 3.0 will show Foo = 2.0 + 3.0 which is not invalid.

feniljain · 2022-05-27T07:58:53Z

I think it is fine to not support float operations for now, since hover on const FOO: f64 = 2.0 + 3.0 will show Foo = 2.0 + 3.0 which is not invalid.

Then ig it makes sense to leave the current tests where they are :)

Veykril · 2022-05-27T12:38:24Z

Thanks!
@bors r+

bors · 2022-05-27T12:38:26Z

📌 Commit 1f4870f has been approved by Veykril

bors · 2022-05-27T12:44:11Z

⌛ Testing commit 1f4870f with merge bd06902...

bors · 2022-05-27T12:51:37Z

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing bd06902 to master...

lnicola · 2022-05-30T03:36:05Z

lnicola reviewed May 26, 2022

View reviewed changes

feniljain force-pushed the fix_bugs branch from 3616b5a to 593ae9e Compare May 26, 2022 12:19

Veykril reviewed May 26, 2022

View reviewed changes

feniljain force-pushed the fix_bugs branch from 593ae9e to 1cc6c29 Compare May 26, 2022 14:01

HKalbasi reviewed May 26, 2022

View reviewed changes

fix: f32 and f64 representation during lowering

1f4870f

feniljain force-pushed the fix_bugs branch from 1cc6c29 to 1f4870f Compare May 26, 2022 14:34

bors merged commit bd06902 into rust-lang:master May 27, 2022

fix: f32 and f64 representation during lowering #12395

fix: f32 and f64 representation during lowering #12395

Uh oh!

Conversation

feniljain commented May 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

feniljain May 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

feniljain May 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

feniljain commented May 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HKalbasi commented May 27, 2022

Uh oh!

feniljain commented May 27, 2022

Uh oh!

Veykril commented May 27, 2022

Uh oh!

bors commented May 27, 2022

Uh oh!

bors commented May 27, 2022

Uh oh!

bors commented May 27, 2022

Uh oh!

lnicola commented May 30, 2022

Uh oh!

Uh oh!

feniljain May 26, 2022 •

edited

Loading

feniljain May 26, 2022 •

edited

Loading

feniljain commented May 27, 2022 •

edited

Loading