Unused arguments warning but they are used

[Zylann]

Godot 3.1 RC1

The following function marks w and h as unused:

static func grid_extract_area_safe_crop(x0, y0, w, h):
	
	var gw = 0
	var gh = 0
	
	if x0 < 0:
		w += x0
		x0 = 0
	if y0 < 0:
		h += y0
		y0 = 0
	
	if x0 + w >= gw:
		w = gw-x0
	if y0 + h >= gh:
		h = gh-y0
	
	return x0 + y0 + w + h

Note: this function makes no sense because I simplified it for repro.

[gsegovia2018]

I had the same issue but I realize that if you give the same value to a new variable works...

static func grid_extract_area_safe_crop(x0, y0, w, h):
	
	var gw = 0
	var gh = 0
	var w1 = w
	var h1 = h
	
	if x0 < 0:
		w1 += x0
		x0 = 0
	if y0 < 0:
		h1 += y0
		y0 = 0
	
	if x0 + w1 >= gw:
		w1 = gw-x0
	if y0 + h1 >= gh:
		h1 = gh-y0
	
	return x0 + y0 + w1 + h1

[aaronfranke]

I'm experiencing this with the 2.5D demo project I'm making. Tested on 3.1.1 stable and latest master.

[Anutrix]

The warning seems to be occurring when a parameter is not visibly used on right side of a =,<,etc. signs.
That is, compound signs are the issues because they don't have variable names on right hand side.
In Zylann's snippet, replacing w += x0 with w = w + x0 fixes it(confirmed).
Same with aaronfranke's, replacing delta /= 3 with delta = delta / 3 should work(unconfirmed).

[Anutrix]

Adding a break at

godot/modules/gdscript/gdscript_parser.cpp

Line 780 in 36591b1

}

seems to fix the issue. Should I make a pull request?
Also, I don't know what exactly does

godot/modules/gdscript/gdscript_parser.cpp

Line 779 in 36591b1

FALLTHROUGH;

do but should I remove it?

[Calinou]

@Anutrix FALLTHROUGH is a macro which is used to denote that a case statement should fall through to the next statement. C++ doesn't implicitly break case statements by default, which is the source of many programmer errors. Therefore, many code style checkers and GCC's -Wextra (which we use on Travis CI) require you to explicitly state your intent to avoid mistakes.

[Anutrix]

Thanks.
Shouldn't comments like

godot/core/packed_data_container.cpp

Line 227 in c8994b5

}; //fallthrough

be replaced by the macro?

@Anutrix FALLTHROUGH is a macro which is used to denote that a case statement should fall through to the next statement. C++ doesn't implicitly break case statements by default, which is the source of many programmer errors. Therefore, many code style checkers and GCC's -Wextra (which we use on Travis CI) require you to explicitly state your intent to avoid mistakes.

[akien-mga]

@Anutrix Yes, seems like I missed this one (and maybe other like that) in #27677.

[Anutrix]

@akien-mga I am sorry. It seems the changes I made in pull request #30095 weren't enough to fix the issue and instead caused another issue.
New issue:

var gl: int
gl += 5

has no warning when it should give (UNASSIGNED_VARIABLE_OP_ASSIGN).
Can you please reopen this issue?
I'll be careful next time.

[Ranoller]

This is not fixed at 13/09/2019 24e1039

[vnen]

I just saw the pull requests and I don't know how they would fix this issue. Well, the first one was reverted, so it's fine. But the second one just made it so assignment with operation mark it as an usage, which just masked the problem.

Assignment is not considered an usage, even the assignment with operations (+=, *=, etc.). This is intended because if you assign a variable multiple times but don't use it elsewhere, you can simply delete the variable (and all the assignments) without changing behavior. Honestly, doing delta = delta / 3 should also not count as an usage, but that's another problem.

I know that the examples here are actually using the arguments, so the issue is valid. I just want to make clear for anyone trying to fix this (before I'm able to) that assignment is not usage.

---

To fix this we need to see why the expressions are not counting up the usage of the arguments.

[name-here]

It doesn't look like anyone has given a clear definition of usage yet, so here's mine:
Usage of a variable means that that variable has some effect on the code, or in other words, is necessary for the code to function as written. This means that, using other values for the variable can cause different things to happen in at least some cases.

From that definition, here are a few things to think about (some more important that others):

• Assigning a value to an object or property passed by reference is definitely usage of the thing passed. All of the following examples are about cases where the variable is not passed.
• Assigning a value to a variable where it will definitely not be used before being cleaned up (ie. between the place where it is last used and the end of the function or other scope in which it was defined) should probably give some sort of warning, though not necessarily one about usage.
• Usage (for normal variables like ints, strings, etc.) includes at least:
  • Using it in an expression that does not only set itself (a += b uses b, while b += b does not)
  • Using it in a Boolean expression that does not only set itself (if a>b: ... uses b, while b = a>b does not, though it probably uses a)
• A few difficult edge cases:
  • If, for example, a is used to set b, but b is not used, it's not really a usage of a. This isn't really too important, since there will be a usage warning for b anyways, but it's something to think about.
  • Places where some complicated function depends on a variable's value, but only effects that same variable don't really constitute usage, but might be very hard to detect (eg. if a > b {b += a} doesn't really use b because, if b is not used to set or determine something else, the variable, along with this if statement, might as well not exist)

[vnen]

My definition of usage (that I used to implement the warning) is quite simple:

• Assignment does not count up the usage. So x = y and x += y does not count a usage of x.
• Any other appearance counts up the usage, removing the warning. So x += x count up the usage because it's on the right side of the assignment.

We can probably polish that up later (e.g. use in self-assignment shouldn't count), but this basic definition should be working first. Missing a unused variable is okay, misreporting a used one is not.

The more complex cases we can leave to the static analyzer.

[name-here]

It seems important to make sure that x = y does in fact count as using x when x is an object. But I agree that the rest is probably less important for now. Also, I just did some testing, and even x = y always counts as usage sometimes on the latest nightly build! I put in the following function, and it fails to find the obvious usage error:

func test_simple( testValue ):
	testValue = 1

It would seem we have to opposite problem in some cases.

[Anutrix]

It seems important to make sure that x = y does in fact count as using x when x is an object. But I agree that the rest is probably less important for now. Also, I just did some testing, and even x = y always counts as usage sometimes on the latest nightly build! I put in the following function, and it fails to find the obvious usage error:

func test_simple( testValue ):
	testValue = 1

It would seem we have to opposite problem in some cases.

It fails because there is indeed no usage if testValue is passed by value(like you said).

In GDScript, only base types (int, float, string and the vector types) are passed by value to functions (value is copied). Everything else (instances, arrays, dictionaries, etc) is passed as reference.

When counting for usage, there's no distinction done by the parser between parameters of base type and that of everything else(currently). The former shouldn't be counted if assigned(or assigned with operations) but the latter should be.
I think this sums up your current point.
Also, I was the one who made those pull requests and I now realize that was bad idea and should be reverted.

[name-here]

I put in the following function, and it fails to find the obvious usage error:

func test_simple( testValue ):
	testValue = 1

It would seem we have to opposite problem in some cases.

It fails because there is indeed no usage if testValue is passed by value(like you said).

What I mean is that the editor does not list an error as it should. It prints an error for:

func test_sinple( testValue ):
  pass

But it does not print an error for

func test_sinple( testValue ):
  testValue = 1

despite these functions being functionally identical.