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

Why does StackTrace.GetFrames() return `StackFrame?[]` instead of `StackFrame[]`? #42422

Closed
jnm2 opened this issue Nov 7, 2019 · 8 comments · Fixed by #42490 or dotnet/coreclr#27787

Comments

@jnm2
Copy link
Collaborator

@jnm2 jnm2 commented Nov 7, 2019

Under what circumstances can the array returned by StackTrace.GetFrames() contain a null element? I couldn't figure out how to write any test that would trigger this behavior.

@jnm2 jnm2 changed the title Why does StackTrace.GetFrames() return StackFrame?[] instead of StackFrame[]? Why does StackTrace.GetFrames() return `StackFrame?[]` instead of `StackFrame[]`? Nov 7, 2019
@danmosemsft

This comment has been minimized.

Copy link
Member

@danmosemsft danmosemsft commented Nov 7, 2019

dotnet/coreclr#23753 (comment)

new StackTrace((StackFrame?)null).GetFrames()[0] == null
@danmosemsft

This comment has been minimized.

Copy link
Member

@danmosemsft danmosemsft commented Nov 7, 2019

Oops, that's not what you asked - you're asking about what GetFrames() returns.

@danmosemsft

This comment has been minimized.

Copy link
Member

@danmosemsft danmosemsft commented Nov 7, 2019

Nope, I was right the first time. Does that answer your question?

@jnm2

This comment has been minimized.

Copy link
Collaborator Author

@jnm2 jnm2 commented Nov 7, 2019

Thank you! That turns this into two more questions.

  1. Why is the StackTrace(StackFrame? frame) constructor parameter annotated as nullable? Would you consider changing in 5.0?

  2. If it must stay nullable, would you consider changing the behavior in 5.0 to:

    public StackTrace(StackFrame? frame)
    {
        (_stackFrames, _numOfFrames) = frame is null
            ? (null, 0)
            : (new StackFrame[] { frame }, 1);
    }
@danmosemsft

This comment has been minimized.

Copy link
Member

@danmosemsft danmosemsft commented Nov 7, 2019

Removing the ? would be a breaking change. We have acknowledged we may need to make such breaking changes, but this may not be enough motivation.

On the face of it your second suggestion sounds reasonable. @stephentoub do you see any reason why it accepts null? Or, conversely, how do you feel about removing the ??

@stephentoub

This comment has been minimized.

Copy link
Member

@stephentoub stephentoub commented Nov 7, 2019

Why is the StackTrace(StackFrame? frame) constructor parameter annotated as nullable?

It's not:
https://github.com/dotnet/coreclr/blob/1b12053ffffc36ad73ad83f7ce111c891c4cf457/src/System.Private.CoreLib/shared/System/Diagnostics/StackTrace.cs#L130

public StackTrace(System.Diagnostics.StackFrame frame) { }

Or, conversely, how do you feel about removing the ??

We should remove the ? from:
https://github.com/dotnet/coreclr/blob/1b12053ffffc36ad73ad83f7ce111c891c4cf457/src/System.Private.CoreLib/shared/System/Diagnostics/StackTrace.cs#L159

public virtual System.Diagnostics.StackFrame?[] GetFrames() { throw null; }

It can contain null, but as far as I can tell only if you violate the contract of the constructor, and if you're explicitly violating one contract, it's fine for others to be "wrong".

@danmosemsft

This comment has been minimized.

Copy link
Member

@danmosemsft danmosemsft commented Nov 8, 2019

@jnm any interest in making the fix mentioned just above? then we can close this.

@jnm2

This comment has been minimized.

Copy link
Collaborator Author

@jnm2 jnm2 commented Nov 8, 2019

Yes, PR incoming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.