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

"When variable type is apparent" doesn't apply to array initialization #26531

Open
davkean opened this issue May 1, 2018 · 18 comments
Open

"When variable type is apparent" doesn't apply to array initialization #26531

davkean opened this issue May 1, 2018 · 18 comments
Assignees
Labels
Milestone

Comments

@davkean
Copy link
Member

davkean commented May 1, 2018

This the same bug I filed #23907, but it stated that it was fixed.

Version Used:
Version 15.8.0 Preview 1.0 [27628.0.d15.8]

Steps to Reproduce:

image

public class Foo
{
    public void Method()
    {
        var files = new string[0];
    }

}

Expected Behavior:

No message

Actual Behavior:

Message IDE0008 Use explicit type instead of 'var' ConsoleApp300 C:\Users\davkean\Source\Repos\ConsoleApp300\ConsoleApp300\Program.cs 6 Active

@davkean
Copy link
Member Author

davkean commented May 1, 2018

tag @jcouv as you originally fixed this.

@sharwell
Copy link
Member

sharwell commented May 1, 2018

This is by design. See #24114.

@sharwell sharwell closed this as completed May 1, 2018
@sharwell sharwell added Resolution-By Design The behavior reported in the issue matches the current design Area-IDE labels May 1, 2018
@sharwell sharwell self-assigned this May 1, 2018
@jcouv
Copy link
Member

jcouv commented May 1, 2018

Specifically, the preference for explicit types on built-in types dominates the preference for var when type is apparent.

@davkean
Copy link
Member Author

davkean commented May 1, 2018

The design is broken: #23714.

@sharwell
Copy link
Member

sharwell commented May 1, 2018

@davkean The design may not match your expectations, but we have no intention of making a universally-configurable system that meets all possible expectations (attempting to do so would doom the entire system to failing for everyone under an impossible testing burden). Some expectations will be broken for each user, but the majority styles are accommodated to a degree deemed reasonably possible.

@davkean
Copy link
Member Author

davkean commented May 1, 2018

"When type is apparent" is one of the options but it doesn't work.

@sharwell
Copy link
Member

sharwell commented May 1, 2018

Do you have a suggestion for different wording that would more accurately reflect the behavior? For example:

When type is apparent, except in cases where the type is a built-in type

Alternately, is there a way to present the three options such that it's clear the set of three items is ordered?

@Neme12
Copy link
Contributor

Neme12 commented May 1, 2018

The current set of options is:

csharp_style_var_for_built_in_types		true/fale
csharp_style_var_when_type_is_apparent	true/false
csharp_style_var_elsewhere				true/false

I believe having these options instead would make sense and only increase the number o possible combinations from 8 to 9 while solving this issue:

csharp_style_var_for_built_in_types		always/never/when_type_is_apparent
csharp_style_var_elsewhere				always/never/when_type_is_apparent

@davkean
Copy link
Member Author

davkean commented May 1, 2018

Yes that makes sense. I'm trying to understand under what circumstances someone would choose the following (which is effectively what options are enforcing now):

csharp_style_var_for_built_in_types		never
csharp_style_var_elsewhere				when_type_is_apparent

I've never seen that style anywhere. Whereas the style I want enforced both project-system and CoreFx/CoreCLR enforce.

@Neme12
Copy link
Contributor

Neme12 commented May 2, 2018

Unfortunately my proposal can't be done in a backwards compatible way because it removes 2 combinations that are currently possible. I created a table mapping current options to the proposed ones:

built_in_types	when_type_is_apparent	elsewhere	|	built_in_types	elsewhere

false		false			false		|	never		never
false		false			true		|	NOT AVAILABLE
false		true			false		|	never		when_type_is_apparent
false		true			true		|	never		always
true		false			false		|	always		never
true		false			true		|	NOT AVAILABLE
true		true			false		|	always		when_type_is_apparent
true		true			true		|	always		always

It would no longer be possible to prefer explicit type when type is apparent but var when it is not apparent. This does seem like a bizarre preference, but even if it was OK to remove this, I don't know how it could be done with editrconfig to map these.

On the other hand, this proposal would add 3 new possible preferences:

	built_in_types			elsewhere

1.	when_type_is_apparent	never
2.	when_type_is_apparent	always
3.	when_type_is_apparent	when_type_is_apparent

where number 3 is the desired one, 2 seems somewhat reasonable and 1 does not.

@Neme12
Copy link
Contributor

Neme12 commented May 2, 2018

@sharwell

It covers things like this:

I don't think so. I would expect 0 to be classified as "not apparent". var would only be suggested if you type var i = new int()

@Neme12
Copy link
Contributor

Neme12 commented May 2, 2018

At least it was my understanding that "apparent" means "the type is explicitly spelled out inside the expression". Maybe I'm wrong.

@davkean
Copy link
Member Author

davkean commented May 2, 2018

@Neme12 That makes sense, I was grouping the following together when I filed #23714, but I like your idea better.

var i = new int[];
var i = 0;

@sharwell
Copy link
Member

sharwell commented May 2, 2018

Reopening to see if the proposal from @Neme12 could provide a suitable replacement for current behavior without being overwhelming.

@sharwell sharwell reopened this May 2, 2018
@sharwell
Copy link
Member

sharwell commented May 2, 2018

@Neme12 The analysis provided comparing the two sets of options is good, but currently misses some of the behaviors. I'm specifically interested in understanding the following cases:

Setting Explicit type was used var was used
always ✔️
never ✔️
when_type_is_apparent ✔️

The current options can represent "allow when type is apparent, and disallow otherwise". I'm concerned the new options would subtly change that to "require when type is apparent, and disallow otherwise".

@sharwell sharwell added this to the Unknown milestone Jun 14, 2018
@sharwell sharwell added Need Design Review The end user experience design needs to be reviewed and approved. and removed Resolution-By Design The behavior reported in the issue matches the current design labels Jun 14, 2018
@JimBobSquarePants
Copy link

Observed behavior indicates that this seems to only trigger for built in types.

With the following config:

csharp_style_var_for_built_in_types = false:warning
csharp_style_var_elsewhere = false:warning
csharp_style_var_when_type_is_apparent = true:warning

Array initialization of a built in type triggers IDE0008 .

var buffer = new byte[n];

image

However custom types do not trigger the error.

var buffer = new MyCustomType[n];

image

@sharwell
Copy link
Member

sharwell commented Feb 5, 2020

I reviewed #26531 (comment) and #26531 (comment) again today with @mavasani. I believe my original concerns can now be addressed by setting the severity for the diagnostic ID instead of setting it as part of the code style configuration. For example, the "allow when apparent, disallow otherwise" setting could be:

csharp_style_var_for_built_in_types = never
csharp_style_var_elsewhere = when_type_is_apparent
# IDE0007: Use implicit type
dotnet_diagnostic.IDE0007.severity = hidden
# IDE0008: Use explicit type
dotnet_diagnostic.IDE0008.severity = warning

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Complete
Development

No branches or pull requests

6 participants