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

No error for unassigned out parameter, bug? #2674

Closed
thomaslevesque opened this issue Jul 21, 2019 · 16 comments
Closed

No error for unassigned out parameter, bug? #2674

thomaslevesque opened this issue Jul 21, 2019 · 16 comments

Comments

@thomaslevesque
Copy link
Member

thomaslevesque commented Jul 21, 2019

Just fixed a bug that was caused by an unassigned out parameter. Normally the compiler emits an error for this, but in this case it doesn't.

This code produces an error, as expected:

private string Test(out string x)
{
	return null;
}

CS0177 The out parameter 'x' must be assigned to before control leaves the current method

Same result when the parameter is int or DateTime.

But this one doesn't produce any error:

private string Test(out PathString x)
{
	return null;
}

(PathString is a readonly struct from Microsoft.AspNetCore.Http)

I also don't get an error if the parameter is a custom struct (e.g. struct Foo {}), whether it's readonly or not.

I'm not sure what's going on here. Why does it produce an error for some types, but not for others?

I get this behavior in C# 7 and 8, didn't check earlier versions.

Is there a logical explanation, or is it a bug in the compiler?

@thomaslevesque
Copy link
Member Author

With a non-empty custom struct, I do get an error, which makes sense, I guess. But it doesn't explain the behavior with PathString, which isn't an empty struct.

@YairHalberstadt
Copy link
Contributor

An empty struct won't error because a struct is considered assigned if all it's fields are assigned, and so an empty struct is always assigned.

@thomaslevesque
Copy link
Member Author

thomaslevesque commented Jul 21, 2019

An empty struct won't error because a struct is considered assigned if all it's fields are assigned, and so an empty struct is always assigned.

Yes, but PathString isn't an empty struct...

@HaloFour
Copy link
Contributor

Are you working with a reference assembly? IIRC there was an issue that because reference structs lacked fields that the compiler could treat them as empty. I forget what the resolution was to resolve that.

@thomaslevesque
Copy link
Member Author

Are you working with a reference assembly?

I think so, yes. I'm using the latest VS2019 Preview and the latest .NET Core 3 SDK.

IIRC there was an issue that because reference structs lacked fields that the compiler could treat them as empty. I forget what the resolution was to resolve that.

I also seem to remember something like that.

@thomaslevesque
Copy link
Member Author

@thomaslevesque
Copy link
Member Author

Related: dotnet/corefx#28870

But this issue is supposed be fixed...

@HaloFour
Copy link
Contributor

If the fix for the issue was recreating the reference assemblies with the dummy field maybe that wasn't done for ASP.NET?

@thomaslevesque
Copy link
Member Author

If the fix for the issue was recreating the reference assemblies with the dummy field maybe that wasn't done for ASP.NET?

Well, the reference definition of PathString does contain the dummy field:
https://github.com/aspnet/AspNetCore/blob/release/3.0-preview7/src/Http/Http.Abstractions/ref/Microsoft.AspNetCore.Http.Abstractions.netcoreapp3.0.cs#L321

And I checked on my machine, the reference assemby for Microsoft.AspNetCore.Http.Abstractions also contains the dummy field. So the problem must be somewhere else...

@yaakov-h
Copy link
Member

Is this a language bug then, or a compiler bug?

@thomaslevesque
Copy link
Member Author

Is this a language bug then, or a compiler bug?

Compiler, I think

@canton7
Copy link

canton7 commented Jul 22, 2019

This probably wants to be in dotnet/roslyn, then, rather than dotnet/csharplang

@thomaslevesque
Copy link
Member Author

This probably wants to be in dotnet/roslyn, then, rather than dotnet/csharplang

Maybe... I don't have the rights to transfer the issue, though.

@svick
Copy link
Contributor

svick commented Aug 9, 2019

As far as I can tell, this has the same source as dotnet/roslyn#29319: Roslyn intentionally ignores private reference type fields in referenced assemblies for purposes of definite assignment, to maintain compatibility.

Though it seems https://github.com/dotnet/roslyn/issues/33288 (of which this issue is a duplicate) indicates this might change in the future.

@333fred
Copy link
Member

333fred commented Aug 9, 2019

As this is a duplicate of a roslyn bug, I'm going to close this bug. If we want to make a language change, we can open one later.

@333fred 333fred closed this as completed Aug 9, 2019
@thomaslevesque
Copy link
Member Author

Works for me. In the meantime I'll use the <Features>strict</Features> flag, which is a good thing anyway.

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

No branches or pull requests

7 participants