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

turbo-tasks: Expand <T as TaskOutput>::Return values in macros #8096

Merged
merged 2 commits into from
May 7, 2024

Conversation

bgw
Copy link
Member

@bgw bgw commented May 7, 2024

Previously, we'd generate function return types like:

<Vc<AssetIdent> as TaskOutput>::Return

which is equivalent to the much simpler:

Vc<AssetIdent>

That approach is nice because it's easier to implement in the macros, and it's more correct, because it uses the type system to verify the type of Vc and Result are what we expect. However, it makes the rustdoc output much harder to read.

This attempts to the expansion of TaskOutput::Return inside the macro to generate more readable documentation, at the cost of some correctness.

Rustdoc Before

Screenshot 2024-05-06 at 5.35.11 PM.png

Rustdoc After

Screenshot 2024-05-06 at 5.35.01 PM.png

Example compilation error with an invalid return type

Screenshot 2024-05-06 at 5.48.00 PM.png

Previously, we'd return types like:

```
<Vc<AssetIdent> as TaskOutput>::Return
```

which is equivalent to the much simpler:

```
Vc<AssetIdent>
```

It's nice because it's easier to implement in the macros, and it's more
correct, because it uses the type system to verify the type of `Vc` and
`Result` are what we expect. However, it makes the rustdoc output hard
to read.

This attempts to the expansion of `TaskOutput::Return` inside the macro
to generate more readable documentation, at the cost of some
correctness.
Copy link

vercel bot commented May 7, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
examples-basic-web 🔄 Building (Inspect) Visit Preview 💬 Add feedback May 7, 2024 2:16am
examples-native-web 🔄 Building (Inspect) Visit Preview 💬 Add feedback May 7, 2024 2:16am
examples-nonmonorepo ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 7, 2024 2:16am
rust-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 7, 2024 2:16am
6 Ignored Deployments
Name Status Preview Comments Updated (UTC)
examples-designsystem-docs ⬜️ Ignored (Inspect) Visit Preview May 7, 2024 2:16am
examples-gatsby-web ⬜️ Ignored (Inspect) Visit Preview May 7, 2024 2:16am
examples-kitchensink-blog ⬜️ Ignored (Inspect) Visit Preview May 7, 2024 2:16am
examples-svelte-web ⬜️ Ignored (Inspect) Visit Preview May 7, 2024 2:16am
examples-tailwind-web ⬜️ Ignored (Inspect) Visit Preview May 7, 2024 2:16am
examples-vite-web ⬜️ Ignored (Inspect) Visit Preview May 7, 2024 2:16am

Copy link
Member Author

bgw commented May 7, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @bgw and the rest of your teammates on Graphite Graphite

Copy link
Contributor

github-actions bot commented May 7, 2024

🟢 Turbopack Benchmark CI successful 🟢

Thanks

Copy link
Contributor

github-actions bot commented May 7, 2024

✅ This change can build next-swc

@bgw bgw changed the title Expand TaskOutput::Return values in the turbo-tasks macros turbo-tasks: Expand TaskOutput::Return values in macros May 7, 2024
Copy link
Contributor

github-actions bot commented May 7, 2024

⚠️ CI failed ⚠️

The following steps have failed in CI:

  • Turbopack Rust tests (mac/win, non-blocking)

See workflow summary for details

@bgw bgw requested review from kwonoj, sokra and wbinnssmith May 7, 2024 00:55
@bgw bgw marked this pull request as ready for review May 7, 2024 00:56
@bgw bgw requested a review from a team as a code owner May 7, 2024 00:57
@bgw bgw marked this pull request as draft May 7, 2024 01:15
Some unit tests used this expansion.
@bgw bgw marked this pull request as ready for review May 7, 2024 02:07
@bgw bgw changed the title turbo-tasks: Expand TaskOutput::Return values in macros turbo-tasks: Expand <T as TaskOutput>::Return values in macros May 7, 2024
@bgw bgw merged commit 7059c03 into main May 7, 2024
48 of 50 checks passed
@bgw bgw deleted the bgw/expand-task-output branch May 7, 2024 19:12
sokra added a commit to vercel/next.js that referenced this pull request May 8, 2024
* vercel/turborepo#8073 <!-- OJ Kwon -
feat(webpack-loaders): support dummy span interface -->
* vercel/turborepo#8083 <!-- OJ Kwon - fix(webpack):
print resource, project path when relative calc fails -->
* vercel/turborepo#8094 <!-- Tim Neutkens -
Implement bindings for Turbopack trace server -->
* vercel/turborepo#8061 <!-- Tobias Koppers - reduce
memory usage in analyser -->
* vercel/turborepo#8077 <!-- Alexander Lyon - Remove
async-trait from a few crates -->
* vercel/turborepo#8102 <!-- Tobias Koppers - fix
memory counting without custom allocator -->
* vercel/turborepo#8096 <!-- Benjamin Woodruff -
turbo-tasks: Expand `<T as TaskOutput>::Return` values in macros -->
* vercel/turborepo#8105 <!-- Benjamin Woodruff -
turbopack-node: Use path.join for postcss loader -->
* vercel/turborepo#8099 <!-- Tim Neutkens - Replace
websocket with tungstenite for turbo-trace-server -->
* vercel/turborepo#8060 <!-- Donny/강동윤 - feat: Add
lint for `grid-template-areas` -->
* vercel/turborepo#8110 <!-- Tobias Koppers - fix
lockfile -->
Neosoulink pushed a commit to Neosoulink/turbo that referenced this pull request Jun 14, 2024
…cel#8096)

Previously, we'd generate function return types like:

```rust
<Vc<AssetIdent> as TaskOutput>::Return
```

which is equivalent to the much simpler:

```rust
Vc<AssetIdent>
```

That approach is nice because it's easier to implement in the macros, and it's more correct, because it uses the type system to verify the type of `Vc` and `Result` are what we expect. However, it makes the rustdoc output much harder to read.

This attempts to the expansion of `TaskOutput::Return` inside the macro to generate more readable documentation, at the cost of some correctness.

<details>
<summary><strong>Rustdoc Before</strong></summary>

![Screenshot 2024-05-06 at 5.35.11 PM.png](https://graphite-user-uploaded-assets-prod.s3.amazonaws.com/HAZVitxRNnZz8QMiPn4a/6a64062d-9fa8-42b9-8788-0972735dfcc8.png)
</details>

<details>
<summary><strong>Rustdoc After</strong></summary>

![Screenshot 2024-05-06 at 5.35.01 PM.png](https://graphite-user-uploaded-assets-prod.s3.amazonaws.com/HAZVitxRNnZz8QMiPn4a/86995431-6e87-4205-ad23-54a2ddb7a535.png)
</details>

<details>
<summary><strong>Example compilation error</strong> with an invalid return type</summary>

![Screenshot 2024-05-06 at 5.48.00 PM.png](https://graphite-user-uploaded-assets-prod.s3.amazonaws.com/HAZVitxRNnZz8QMiPn4a/257cb449-4b8e-4fde-8ee0-0b2af1b7b435.png)
</details>
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 25, 2024
…cel/turborepo#8096)

Previously, we'd generate function return types like:

```rust
<Vc<AssetIdent> as TaskOutput>::Return
```

which is equivalent to the much simpler:

```rust
Vc<AssetIdent>
```

That approach is nice because it's easier to implement in the macros, and it's more correct, because it uses the type system to verify the type of `Vc` and `Result` are what we expect. However, it makes the rustdoc output much harder to read.

This attempts to the expansion of `TaskOutput::Return` inside the macro to generate more readable documentation, at the cost of some correctness.

<details>
<summary><strong>Rustdoc Before</strong></summary>

![Screenshot 2024-05-06 at 5.35.11 PM.png](https://graphite-user-uploaded-assets-prod.s3.amazonaws.com/HAZVitxRNnZz8QMiPn4a/6a64062d-9fa8-42b9-8788-0972735dfcc8.png)
</details>

<details>
<summary><strong>Rustdoc After</strong></summary>

![Screenshot 2024-05-06 at 5.35.01 PM.png](https://graphite-user-uploaded-assets-prod.s3.amazonaws.com/HAZVitxRNnZz8QMiPn4a/86995431-6e87-4205-ad23-54a2ddb7a535.png)
</details>

<details>
<summary><strong>Example compilation error</strong> with an invalid return type</summary>

![Screenshot 2024-05-06 at 5.48.00 PM.png](https://graphite-user-uploaded-assets-prod.s3.amazonaws.com/HAZVitxRNnZz8QMiPn4a/257cb449-4b8e-4fde-8ee0-0b2af1b7b435.png)
</details>
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 29, 2024
…cel/turborepo#8096)

Previously, we'd generate function return types like:

```rust
<Vc<AssetIdent> as TaskOutput>::Return
```

which is equivalent to the much simpler:

```rust
Vc<AssetIdent>
```

That approach is nice because it's easier to implement in the macros, and it's more correct, because it uses the type system to verify the type of `Vc` and `Result` are what we expect. However, it makes the rustdoc output much harder to read.

This attempts to the expansion of `TaskOutput::Return` inside the macro to generate more readable documentation, at the cost of some correctness.

<details>
<summary><strong>Rustdoc Before</strong></summary>

![Screenshot 2024-05-06 at 5.35.11 PM.png](https://graphite-user-uploaded-assets-prod.s3.amazonaws.com/HAZVitxRNnZz8QMiPn4a/6a64062d-9fa8-42b9-8788-0972735dfcc8.png)
</details>

<details>
<summary><strong>Rustdoc After</strong></summary>

![Screenshot 2024-05-06 at 5.35.01 PM.png](https://graphite-user-uploaded-assets-prod.s3.amazonaws.com/HAZVitxRNnZz8QMiPn4a/86995431-6e87-4205-ad23-54a2ddb7a535.png)
</details>

<details>
<summary><strong>Example compilation error</strong> with an invalid return type</summary>

![Screenshot 2024-05-06 at 5.48.00 PM.png](https://graphite-user-uploaded-assets-prod.s3.amazonaws.com/HAZVitxRNnZz8QMiPn4a/257cb449-4b8e-4fde-8ee0-0b2af1b7b435.png)
</details>
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 29, 2024
…cel/turborepo#8096)

Previously, we'd generate function return types like:

```rust
<Vc<AssetIdent> as TaskOutput>::Return
```

which is equivalent to the much simpler:

```rust
Vc<AssetIdent>
```

That approach is nice because it's easier to implement in the macros, and it's more correct, because it uses the type system to verify the type of `Vc` and `Result` are what we expect. However, it makes the rustdoc output much harder to read.

This attempts to the expansion of `TaskOutput::Return` inside the macro to generate more readable documentation, at the cost of some correctness.

<details>
<summary><strong>Rustdoc Before</strong></summary>

![Screenshot 2024-05-06 at 5.35.11 PM.png](https://graphite-user-uploaded-assets-prod.s3.amazonaws.com/HAZVitxRNnZz8QMiPn4a/6a64062d-9fa8-42b9-8788-0972735dfcc8.png)
</details>

<details>
<summary><strong>Rustdoc After</strong></summary>

![Screenshot 2024-05-06 at 5.35.01 PM.png](https://graphite-user-uploaded-assets-prod.s3.amazonaws.com/HAZVitxRNnZz8QMiPn4a/86995431-6e87-4205-ad23-54a2ddb7a535.png)
</details>

<details>
<summary><strong>Example compilation error</strong> with an invalid return type</summary>

![Screenshot 2024-05-06 at 5.48.00 PM.png](https://graphite-user-uploaded-assets-prod.s3.amazonaws.com/HAZVitxRNnZz8QMiPn4a/257cb449-4b8e-4fde-8ee0-0b2af1b7b435.png)
</details>
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Aug 1, 2024
…cel/turborepo#8096)

Previously, we'd generate function return types like:

```rust
<Vc<AssetIdent> as TaskOutput>::Return
```

which is equivalent to the much simpler:

```rust
Vc<AssetIdent>
```

That approach is nice because it's easier to implement in the macros, and it's more correct, because it uses the type system to verify the type of `Vc` and `Result` are what we expect. However, it makes the rustdoc output much harder to read.

This attempts to the expansion of `TaskOutput::Return` inside the macro to generate more readable documentation, at the cost of some correctness.

<details>
<summary><strong>Rustdoc Before</strong></summary>

![Screenshot 2024-05-06 at 5.35.11 PM.png](https://graphite-user-uploaded-assets-prod.s3.amazonaws.com/HAZVitxRNnZz8QMiPn4a/6a64062d-9fa8-42b9-8788-0972735dfcc8.png)
</details>

<details>
<summary><strong>Rustdoc After</strong></summary>

![Screenshot 2024-05-06 at 5.35.01 PM.png](https://graphite-user-uploaded-assets-prod.s3.amazonaws.com/HAZVitxRNnZz8QMiPn4a/86995431-6e87-4205-ad23-54a2ddb7a535.png)
</details>

<details>
<summary><strong>Example compilation error</strong> with an invalid return type</summary>

![Screenshot 2024-05-06 at 5.48.00 PM.png](https://graphite-user-uploaded-assets-prod.s3.amazonaws.com/HAZVitxRNnZz8QMiPn4a/257cb449-4b8e-4fde-8ee0-0b2af1b7b435.png)
</details>
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Aug 16, 2024
* vercel/turborepo#8073 <!-- OJ Kwon -
feat(webpack-loaders): support dummy span interface -->
* vercel/turborepo#8083 <!-- OJ Kwon - fix(webpack):
print resource, project path when relative calc fails -->
* vercel/turborepo#8094 <!-- Tim Neutkens -
Implement bindings for Turbopack trace server -->
* vercel/turborepo#8061 <!-- Tobias Koppers - reduce
memory usage in analyser -->
* vercel/turborepo#8077 <!-- Alexander Lyon - Remove
async-trait from a few crates -->
* vercel/turborepo#8102 <!-- Tobias Koppers - fix
memory counting without custom allocator -->
* vercel/turborepo#8096 <!-- Benjamin Woodruff -
turbo-tasks: Expand `<T as TaskOutput>::Return` values in macros -->
* vercel/turborepo#8105 <!-- Benjamin Woodruff -
turbopack-node: Use path.join for postcss loader -->
* vercel/turborepo#8099 <!-- Tim Neutkens - Replace
websocket with tungstenite for turbo-trace-server -->
* vercel/turborepo#8060 <!-- Donny/강동윤 - feat: Add
lint for `grid-template-areas` -->
* vercel/turborepo#8110 <!-- Tobias Koppers - fix
lockfile -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants