Skip to content

fix: f32 and f64 representation during lowering #12395

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

Merged
merged 1 commit into from
May 27, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions crates/hir-def/src/body/lower.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ use crate::{
builtin_type::{BuiltinFloat, BuiltinInt, BuiltinUint},
db::DefDatabase,
expr::{
dummy_expr_id, Array, BindingAnnotation, Expr, ExprId, Label, LabelId, Literal, MatchArm,
Pat, PatId, RecordFieldPat, RecordLitField, Statement,
dummy_expr_id, Array, BindingAnnotation, Expr, ExprId, FloatTypeWrapper, Label, LabelId,
Literal, MatchArm, Pat, PatId, RecordFieldPat, RecordLitField, Statement,
},
intern::Interned,
item_scope::BuiltinShadowMode,
Expand Down Expand Up @@ -968,7 +968,10 @@ impl From<ast::LiteralKind> for Literal {
// FIXME: these should have actual values filled in, but unsure on perf impact
LiteralKind::IntNumber(lit) => {
if let builtin @ Some(_) = lit.suffix().and_then(BuiltinFloat::from_suffix) {
Literal::Float(Default::default(), builtin)
Literal::Float(
FloatTypeWrapper::new(lit.float_value().unwrap_or(Default::default())),
builtin,
)
} else if let builtin @ Some(_) = lit.suffix().and_then(BuiltinInt::from_suffix) {
Literal::Int(lit.value().unwrap_or(0) as i128, builtin)
} else {
Expand All @@ -978,7 +981,7 @@ impl From<ast::LiteralKind> for Literal {
}
LiteralKind::FloatNumber(lit) => {
let ty = lit.suffix().and_then(BuiltinFloat::from_suffix);
Literal::Float(Default::default(), ty)
Literal::Float(FloatTypeWrapper::new(lit.value().unwrap_or(Default::default())), ty)
}
LiteralKind::ByteString(bs) => {
let text = bs.value().map(Box::from).unwrap_or_else(Default::default);
Expand Down
23 changes: 22 additions & 1 deletion crates/hir-def/src/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,24 @@ pub struct Label {
}
pub type LabelId = Idx<Label>;

// 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 {
pub 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))
}
}

#[derive(Debug, Clone, Eq, PartialEq)]
pub enum Literal {
String(Box<str>),
Expand All @@ -46,7 +64,10 @@ pub enum Literal {
Bool(bool),
Int(i128, Option<BuiltinInt>),
Uint(u128, Option<BuiltinUint>),
Float(u64, Option<BuiltinFloat>), // FIXME: f64 is not Eq
// Here we are using a wrapper around float because f32 and f64 do not implement Eq, so they
// could not be used directly here, to understand how the wrapper works go to definition of
// FloatTypeWrapper
Float(FloatTypeWrapper, Option<BuiltinFloat>),
}

#[derive(Debug, Clone, Eq, PartialEq)]
Expand Down
66 changes: 66 additions & 0 deletions crates/ide/src/hover/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3595,6 +3595,72 @@ const FOO$0: u8 = b'\x61';

---

This is a doc
"#]],
);
// show float literal
check(
r#"
/// This is a doc
const FOO$0: f64 = 1.0234;
"#,
expect![[r#"
*FOO*

```rust
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@feniljain feniljain May 26, 2022

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

test
```

```rust
const FOO: f64 = 1.0234
```

---

This is a doc
"#]],
);
//show float typecasted from int
check(
r#"
/// This is a doc
const FOO$0: f32 = 1f32;
"#,
expect![[r#"
*FOO*

```rust
test
```

```rust
const FOO: f32 = 1
```

---

This is a doc
"#]],
);
//show f64 typecasted from float
check(
r#"
/// This is a doc
const FOO$0: f64 = 1.0f64;
"#,
expect![[r#"
*FOO*

```rust
test
```

```rust
const FOO: f64 = 1
```

---

This is a doc
"#]],
);
Expand Down
42 changes: 37 additions & 5 deletions crates/syntax/src/ast/token_ext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -319,17 +319,49 @@ impl ast::IntNumber {
Some(suffix)
}
}

pub fn float_value(&self) -> Option<f64> {
let (_, text, _) = self.split_into_parts();
text.parse::<f64>().ok()
}
}

impl ast::FloatNumber {
pub fn suffix(&self) -> Option<&str> {
pub fn split_into_parts(&self) -> (&str, &str) {
let text = self.text();
let mut float_text = self.text();
let mut suffix = "";
let mut indices = text.char_indices();
let (mut suffix_start, c) = indices.by_ref().find(|(_, c)| c.is_ascii_alphabetic())?;
if c == 'e' || c == 'E' {
suffix_start = indices.find(|(_, c)| c.is_ascii_alphabetic())?.0;
if let Some((mut suffix_start, c)) = indices.by_ref().find(|(_, c)| c.is_ascii_alphabetic())
{
if c == 'e' || c == 'E' {
if let Some(suffix_start_tuple) = indices.find(|(_, c)| c.is_ascii_alphabetic()) {
suffix_start = suffix_start_tuple.0;

float_text = &text[..suffix_start];
suffix = &text[suffix_start..];
}
} else {
float_text = &text[..suffix_start];
suffix = &text[suffix_start..];
}
}
Some(&text[suffix_start..])

(float_text, suffix)
}

pub fn suffix(&self) -> Option<&str> {
let (_, suffix) = self.split_into_parts();
if suffix.is_empty() {
None
} else {
Some(suffix)
}
}

pub fn value(&self) -> Option<f64> {
let (text, _) = self.split_into_parts();
text.parse::<f64>().ok()
}
}

Expand Down