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

feat(core): #[command] return with autoref specialization workaround fix #1672 #1734

Merged
merged 9 commits into from
May 9, 2021

Conversation

chippers
Copy link
Member

@chippers chippers commented May 6, 2021

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • New Binding Issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • No

The PR fulfills these requirements:

  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix: #xxx[,#xxx], where "xxx" is the issue number)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:
So we can't really return a trait while also having a default implementation for Serialize because Result is also Serialize if the contents are. Since the contents need to be Serialize to be useful as a command result, the implementations would conflict if we tried to have a specific implementation for Result. If we don't have a specific implementation for result, the value would be sent as Result<Result<T, E>, InvokeError>.

This is not the only solution, but is the one with the least user-land code needed. An alternative would be to manually impl every type that Serialize does for a CommandResult trait, and also require the user to implement it for any types they have that are serializable that they wish to return from commands. Not ideal, but possible.

@chippers chippers requested review from a team May 6, 2021 21:32
@chippers chippers changed the title feat(core): #[command] return with autoref specialization workaround feat(core): #[command] return with autoref specialization workaround fix #1672 May 6, 2021
@chippers chippers marked this pull request as draft May 6, 2021 21:51
@chippers
Copy link
Member Author

chippers commented May 6, 2021

marked as draft until we decide this is the solution we want to use

async + elided borrow + no return = error
fix needed for that use case (e.g. using `State<'_, T>`)
@chippers
Copy link
Member Author

chippers commented May 8, 2021

Note: The simple async state command is disabled for now until I look at it again tomorrow. Because async functions add + '_ to the resulting future, this command expects State<'_, MyState> to live until `'static' because that is the lifetime of the resulting future.

Either a way to specify that lifetime better is needed in the underlying code, or a documented workaround to specify that type is needed.

@chippers
Copy link
Member Author

chippers commented May 8, 2021

intellij-rust throws a false positive when using async in #[command(async)]. Since async is preferable, it will be kept for now.

If intellij-rust/intellij-rust#7214 is closed as wont-fix, then we should add another option to be used. e.g. future

@chippers
Copy link
Member Author

chippers commented May 9, 2021

This PR is ready to review - all commands that existed before it work.

We should probably make sure that the commands example is fully fleshed out, and that every command in rust has a button on the frontend - but that can wait for another PR.

The autoref specialization workaround is what lets us "unwrap" the result of the function if it happens to be Result<impl Serialize, impl Into<InvokeError>>. Without it, the results themselves would be serialized and the variants would be exposed to the sent Ok and Err values. You can tell if this happens with the commands example because the result will be [object Object]. None of the current commands in that example should result in that.

@chippers chippers marked this pull request as ready for review May 9, 2021 00:43
@chippers chippers requested a review from a team as a code owner May 9, 2021 11:50
@lucasfernog lucasfernog merged commit bb8dafb into dev May 9, 2021
@lucasfernog lucasfernog deleted the feat/command-return branch May 9, 2021 11:52
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.

None yet

2 participants