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

cgen: fix codegen for option on concatexpr #22964

Merged
merged 2 commits into from
Nov 25, 2024

Conversation

felipensp
Copy link
Member

@felipensp felipensp commented Nov 24, 2024

@Delta456
Copy link
Member

This shouldn't be working as the language already disallows !?type. We also discussed this in discord.

@spytheman
Copy link
Member

This shouldn't be working as the language already disallows !?type. We also discussed this in discord.

This is not !?Type however.

It is a result tuple, that just happens to contain an option type. Option type values should have first class support in V, and so should be able to be used everywhere other primitive types are allowed, including in result tuples.

@spytheman
Copy link
Member

The problem with !?Type, is that it has 3 possible kinds of values, while the language has constructs like or blocks and if x := f() {, that can deal well with 2 at a time, and it is very confusing to use them, when there are 3 - one of those 3 will get forgotten, or cause bugs.

The only construct that can deal with 3 at the same time, is match x {, but that is very verbose to be used in all places that universal support of the old ?!Type combination would need.

For result tuples/multiple result values, with options inside them, there are no such problems, because the outer result, will get handled right away in one construct, and then what remains will be a normal option value, that can then be handled with the usual V constructs:

fn f() !(int, ?int) { return 1,2 }

a,b := f()!
// a will be `int(1)` and b will be `?int(2)`, and you can do `b or { ... }` etc

Copy link
Member

@spytheman spytheman left a comment

Choose a reason for hiding this comment

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

Excellent work.

@spytheman spytheman marked this pull request as ready for review November 25, 2024 00:17
@spytheman spytheman merged commit 904bccb into vlang:master Nov 25, 2024
72 checks passed
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.

Wrong behavior of !(Type, ?Type) result tuple
3 participants