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

[naga wgsl-in] Allow abstract literals to be used as return values #7035

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

jamienicol
Copy link
Contributor

@jamienicol jamienicol commented Jan 30, 2025

Description

Currently we concretize() return values via calling expression(), meaning this works fine:

fn foo() -> i32 {
    return 1;
}

as we concretize the 1 to an i32, which is the function return type. But this does not:

fn foo() -> f32 {
    return 1;
}

as we concretize the 1 to an i32, which does not match the f32 return type.

We can fix this by calling expression_for_abstract() which does not concretize, then calling try_automatic_conversions() to attempt to convert the abstract type to the return type.

Testing
Added some snapshot tests

Checklist

  • Run cargo fmt.
  • Run cargo clippy. If applicable, add:
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

@jamienicol jamienicol marked this pull request as draft January 30, 2025 16:39
@jamienicol jamienicol force-pushed the abstract-return branch 3 times, most recently from 8b6fa54 to 1405042 Compare January 30, 2025 18:49
Comment on lines +1687 to +1692
let mut ctx = ctx.as_expression(block, &mut emitter);
let mut expr_ty = resolve_inner!(ctx, expr);
while let crate::TypeInner::Array { base, .. } = *expr_ty {
expr_ty = &ctx.module.types[base].inner;
}
if expr_ty.scalar().is_some_and(|s| s.is_abstract()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jimblandy I'm avoiding calling try_automatic_conversions() here for non-abstract types, as otherwise I saw in one of the tests that this code:

@group(0) @binding(0)
var<storage> atom: atomic<u32>;

fn return_atomic() -> atomic<u32> {
  return atom;
}

used to fail with this error:

  ┌─ in.wgsl:4:1
  │
4 │ ╭ fn return_atomic() -> atomic<u32> {
5 │ │   return atom;
  │ ╰──────────────^ naga::Function [0]
  │
  = The function's given return type cannot be returned from functions

but would now fail with:

Could not parse WGSL:
error: automatic conversions cannot convert `u32` to `atomic<u32>`
  ┌─ in.wgsl:4:10
  │
4 │   return atom;
  │          ^^^^ this expression has type u32

which is less clear.

Would it make sense to have this check as part of try_automatic_conversions()? Do we ever convert from things that are not abstract?

@jamienicol jamienicol marked this pull request as ready for review February 1, 2025 07:22
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.

1 participant