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

v: add $res compile time function to get returned value in defer block #18382

Conversation

LouisSchmieder
Copy link
Member

@LouisSchmieder LouisSchmieder commented Jun 8, 2023

This PR add the feature to get the returned value of the function inside a defer block. This PR only adds this feature for functions, which return one or more values; Results are not included.

// prints 41
fn foo() int {
	defer {
		println($res())
	}
	return 41
}

// prints 30
fn multi() (int, int) {
	defer {
		println($res() * $res())
	}
	return 5, 6
}

I added this feature, since it can be very helpful in a situation like this:

pub fn (mut web Web) check_auth() bool {
	auth_header := web.get_header('Authorization').split(' ')
	mut authorized := false
	defer {
		if !authorized {
			web.set_status(401, 'Unauthorized')
			web.ok('')
		}
	}
	if auth_header.len == 0 {
		return false
	}
	eprintln(auth_header)
	match auth_header[0] {
		'Bearer' {
			if auth_header.len == 2 {
				authorized = auth_header[1] == 'test'
				return authorized
			}
			else { return false }
		}
		else { return false }
	}
	authorized = true
}

It can be simplified to this:

pub fn (mut web Web) check_auth() bool {
	auth_header := web.get_header('Authorization').split(' ')
	defer {
		if !$res() {
			web.set_status(401, 'Unauthorized')
			web.ok('')
		}
	}
	if auth_header.len == 0 {
		return false
	}
	eprintln(auth_header)
	match auth_header[0] {
		'Bearer' {
			if auth_header.len == 2 {
				return auth_header[1] == 'test'
			}
			else { return false }
		}
		else { return false }
	}
}

And when the complexity of this function increases, it gets even more helpful, since there is no additional variables handling for the result.

🤖 Generated by Copilot at 39bbc24

This pull request adds support for the res function, which allows accessing the returned value of a function inside a defer block. It modifies the parser, checker, formatter, and generator modules to handle the res function in defer statements and comptime calls. It also adds several test files to verify the correct behavior and error handling of the res function.

🤖 Generated by Copilot at 39bbc24

  • Add a new comptime function res that allows accessing the returned value of a function inside a defer block (link, link, link, link, link, link)
  • Add a new field fn_return_type to the Checker struct to store the return type of the current function being checked (link)
  • Assign the return type of the function node to the fn_return_type field of the checker in the Checker.check_fn_decl method (link)
  • Add a new field defer_return_tmp_var to the Gen struct to store the name of the temporary variable that holds the returned value of a function (link)
  • Assign the name of the temporary variable to the defer_return_tmp_var field of the generator in the Gen.gen_fn_decl method (link)
  • Write the name of the temporary variable with or without the index argument in the generated C code for the res function call (link)
  • Return the temporary variable after writing the defer statements and autofreeing the scope variables in the generated C code for the function (link, link, link, link, link, link)
  • The function must be inside a defer block (link)
  • The function must return something (link)
  • The function must not return a Result type (link)
  • The function must return a single value or the index of the desired value must be provided (link)
  • Add test files for the res function usage and error cases in the vlib/v/checker/tests and vlib/v/tests directories (link, link, link, link, link, link, link, link, link, link, link)

@spytheman
Copy link
Member

spytheman commented Jun 10, 2023

I can see the utility of something like that, but I am afraid, that it would prevent our defer {} implementation to be changed later, to be executed at the end of the current scope, not just at the end of the current function ($res() will have no assigned value in the case of a scope).

To me, having a defer {} that will be executed at the end of the current scope in the future, has much more utility than having the current defer {} even if it is enhanced with the ability to access $res(), since it will make freeing of allocated resources in loops much easier/intuitive/predictable.

Potentially, we could have both defer kinds, if there is a syntax for distinguishing the 2 kinds of defer (function scoped defer, and lexical scope defer), but that is a decision that @medvednikov should make.

@spytheman
Copy link
Member

Unrelated to my other comment, this new feature does need documentation in doc/docs.md and CHANGELOG.md, if it is going to be merged.

@medvednikov
Copy link
Member

@spytheman we're most likely going to have 2 defers: one for the end of the block, and one for the end of the function.

@Casper64
Copy link
Member

I can see the use of a defer that can check the result. I've heard from others that there is a need of some sort of "after" evaluation especially in the context of vweb's middleware (I assume that is what your example represents).

But in your case the code can be simplified by changing the control flow a bit.

pub fn (mut web Web) check_auth() bool {
	auth_header := web.get_header('Authorization').split(' ')
	eprintln(auth_header)

	if auth_header.len == 2 && auth_header[0] == 'Bearer' && auth_header[1] == 'test' {
		return true
	} else {
		web.set_status(401, 'Unauthorized')
		web.ok('')
		return false
	}
}

Because C evaluates expressions lazely and we first verify that auth_header is of length 2 this code won't fail.
I agree that in many cases this will be a useful feature, but the code does get more complicated.

@LouisSchmieder
Copy link
Member Author

I see what you mean @spytheman. But now, since there will be 2 different defer statements, what happens with this merge request, because the feature isn't a huge mess of code; it could be merged (and obviously I would add docs and changelog) or the feature will come at another time, when the differentiation of the defer statements is done (which is a bit too much for me, while I don't have that much time for it).

@ArtemkaKun ArtemkaKun added the Needs Rebase The code of the PR must be rebased over current master before it can be approved. label Jun 13, 2023
@LouisSchmieder LouisSchmieder removed the Needs Rebase The code of the PR must be rebased over current master before it can be approved. label Jun 13, 2023
@medvednikov
Copy link
Member

I still don't fully understand the need, does any other language have something similar?

@LouisSchmieder
Copy link
Member Author

In go there is a feature, which uses named return values (https://riandyrn.medium.com/golang-magic-modify-return-value-using-deferred-function-ed0eabdaa75), which also allows modifying the value in the defer statement, as well as use it directly; e.g.

func test1() (i int) {
	defer func() { i++ }()
	return 1
}

@spytheman
Copy link
Member

I still don't fully understand the need, does any other language have something similar?

The utility is if you need to quickly/conveniently do some processing/logging of the result value of a function, without having to refactor the existing code first (that used multiple return lines, potentially with different expressions each), to use a mut res := ... res = expr return res pattern.

@spytheman
Copy link
Member

But now, since there will be 2 different defer statements, what happens with this merge request, because the feature isn't a huge mess of code; it could be merged (and obviously I would add docs and changelog)

@LouisSchmieder given that @medvednikov agreed that there will be 2 variants of defer (i.e. that a scoped defer will be possible in the future), I do not have objections to merging that on technical grounds. The feature just needs docs.

doc/docs.md Outdated Show resolved Hide resolved
@medvednikov medvednikov merged commit dbd2517 into vlang:master Jun 17, 2023
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.

5 participants