Skip to content

[BUG] Analysis of uninitialized local fails at if without else #440

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

Closed
realgdman opened this issue May 8, 2023 · 5 comments
Closed

[BUG] Analysis of uninitialized local fails at if without else #440

realgdman opened this issue May 8, 2023 · 5 comments
Labels
bug Something isn't working

Comments

@realgdman
Copy link

Analysis of uninitialized local fails to see initialization which happens after single-if without else.

Reproduce Code

foo: (x: bool) -> int = {
  i: int;
  if(x) { 
    i = 1;
    return i; 
  }
  i = 2;
  return i;
}

main: () = foo(true);

Version
latest
Command line
cppfront/cppfront $1.cpp2 -p
clang++-15 -Icppfront/include $1.cpp -std=c++20 -o $1

Expected result
Compilation success

Actual result/error

initpath.cpp2(2,2): error: local variable i must be initialized on both branches or neither branch
initpath.cpp2(3,2): error: "if" initializes i on:
  branch starting at line 3
but not on:
  implicit else branch

Additional context
With return, both paths init and return.
Without return, there is double initialization, but error is the same.

@DyXel
Copy link
Contributor

DyXel commented Jun 17, 2024

Shorter case:

my_call: () -> (result: int) = {
	if true {
		result = EXIT_SUCCESS;
		return;
	}
	result = EXIT_FAILURE;
}

Outputs:

test.cpp2...
test.cpp2(1,17): error: local variable result must be initialized on both branches or neither branch
test.cpp2(2,2): error: "if" initializes result on:
  branch starting at line 2
but not on:
  implicit else branch
  ==> program violates initialization safety guarantee - see previous errors

I'd expect this to just work since whatever comes after the if statement is a "implicit else branch" (in my eyes at least).

@gregmarr
Copy link
Contributor

I'd expect this to just work since whatever comes after the if statement is a "implicit else branch" (in my eyes at least).

As far as that analysis is concerned, your function looks like this, and that is the implicit else that it's referring to:

    if true {
        result = EXIT_SUCCESS;
        return;
    } else {
    }
    result = EXIT_FAILURE;

The requirement is currently that it must be assigned on every branch. The analysis would have to be a lot more sophisticated to determine that the if branch always returns, and thus the result = EXIT_FAILURE is an initialization, and not an assignment.

In this case, the EXIT_FAILURE line is an assignment if something is true, and an initialization if something is false.

    if something {
        result = EXIT_SUCCESS;
    } else {
    }
    result = EXIT_FAILURE;

Here's one that's more difficult to analyze:

    if something {
        result = EXIT_SUCCESS;
        if other {
            return;
        }
    } else {
    }
    result = EXIT_FAILURE;

@DyXel
Copy link
Contributor

DyXel commented Jun 17, 2024

Yes, I understand why it happens (thanks for writing down the explanation in any case, for other readers). But I still consider this a bug, for ergonomic reasons:

It's pretty common to write code that has the happy case in the first if statement at the beginning of the function, and roll out the rest of the logic in (possibly) chain of extra if-statements. So I feel like at least the basic cases that consider this scenario ought to work, because that's a common way to start writing code.

@gregmarr
Copy link
Contributor

Agreed, I'm just saying that it would be good for a final version of this functionality to support that, but due to the complexities involved, I understand why the current state is the way it is.

@hsutter
Copy link
Owner

hsutter commented Jun 20, 2024

Thanks! This is intentional... the analysis doesn't do any control flow or data flow beyond checking that branch blocks do/don't initialize the local.

Briefly, what is subtle to teach the compiler about will also be subtle for the human reading the code. Examples like this could be allowed, but start to get higher-cost (subtler to read/maintain) and also lower-benefit (occur less often). For example, it would have to take into account whether all possibly-nested branches in a branch tree do an early return.

I understand the request though, I get similar things all the time with the C++ Core Guidelines Pro.Lifetime static analysis... a very common request is to support code like this:

// Cpp1
X* f() {
    X* p = nullptr;
    if (something) {
        p = new X;
    }
    // ... arbitrary quantities of code that doesn't use p
    if (something) {
        p->foo();  // error, p might be null; but people request this not to be an error
    }
    return p;
}

The human can reason that p is only dereferenced if allocated because both paths are "guarded" by if (something) and the user knows something is idempotent. In fact, something might even be a const value flag that the compiler can prove won't change. But it's a very slippery slope to "well if you support this I have his next-slightly-less-simple case that's also pretty simple"... the lifetime diagnosis and initialization are intended to enable common good clear uses, not every simple use.

That said, the day may come where I support at least simple uses like if (const_bool_var) in code like the above for the Lifetime analysis, or return from blocks. I'll keep the idea on the backlog for the future, but I'll leave it as it for today.

Thanks for the suggestion though, and thanks for understanding!

@hsutter hsutter closed this as completed Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants