fix: f32 and f64 representation during lowering #12395

Conversation

Contributor

feniljain commented 7 days ago

should fix #12380

jhgg and joshsleeper reacted with heart emoji

Outdated


		#[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.

Contributor

Author

feniljain 7 days ago

Using tuple struct seemed good, fixed the nit too!

Contributor

Author

feniljain 7 days ago •

edited

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?

crates/syntax/src/ast/token_ext.rs

Outdated

		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

Contributor

Author

feniljain 7 days ago

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

crates/syntax/src/ast/token_ext.rs

Outdated

		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

lnicola reacted with thumbs up emoji

Contributor

Author

feniljain 7 days ago

Made changes for the same

crates/syntax/src/ast/token_ext.rs

Outdated

		@@ -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).

Contributor

Author

HKalbasi 7 days ago

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

Contributor

Author

feniljain 7 days ago •

edited

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

Contributor

HKalbasi 7 days ago

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.

Contributor

Author

feniljain commented 6 days ago •

edited

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:

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

Contributor

HKalbasi commented 6 days ago

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.

Contributor

Author