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

GetTypeInfo().Type should return null for default literal #31029

Closed
jcouv opened this issue Nov 7, 2018 · 3 comments · Fixed by #37596
Closed

GetTypeInfo().Type should return null for default literal #31029

jcouv opened this issue Nov 7, 2018 · 3 comments · Fixed by #37596
Assignees
Labels
4 - In Review A fix for the issue is submitted for review. Area-Compilers
Milestone

Comments

@jcouv
Copy link
Member

jcouv commented Nov 7, 2018

From Neal (in #26027):

I think both the spec and design notes are clear about the language intent: default is typeless and gets a type on conversion.

        [Fact]
        public void AssignmentToInt()
        {
            string source = @"
class C
{
    static void Main()
    {
        int x = default;
        System.Console.Write(x);
    }
}
";
            var comp = CreateCompilation(source, parseOptions: TestOptions.Regular7_1, options: TestOptions.DebugExe);
...

            var tree = comp.SyntaxTrees.First();
            var model = comp.GetSemanticModel(tree);
            var nodes = tree.GetCompilationUnitRoot().DescendantNodes();

            var def = nodes.OfType<LiteralExpressionSyntax>().Single();
            Assert.Equal("System.Int32", model.GetTypeInfo(def).Type.ToTestDisplayString()); // should be null
            Assert.Equal("System.Int32", model.GetTypeInfo(def).ConvertedType.ToTestDisplayString());
...
        }
@Neme12
Copy link
Contributor

Neme12 commented Nov 7, 2018

I like this but I just wanted to point out that this seems to have been an intentional decision after @AlekseyTs stated his concerns here: #17205 (comment) arguing that GetTypeInfo().Type should match the type of the constant value. I think an ideal scenario would be that the Type was null and constant value didn't exist, while there would be another API for GetConvertedConstantValue (or something like GetConstantValueInfo().ConvertedValue) which would match the ConvertedType and would be present on default literals.

Such an API could be useful on its own because it seems this functionality is currently missing. For example someone might want to know that the converted constant value of 'A' in 'A' + 1 is 65.

The only concern with this is that removing the constant value from a default literal could be a huge breaking change. However removing the type will be a breaking change too, breaking a lot of IDE code I imagine. (Actually, now that I'm thinking about it, removing the type will be a much larger break, I can't think of a scenario where IDE uses the constant value).

also tagging @gafter for thoughts

@Neme12
Copy link
Contributor

Neme12 commented Dec 16, 2018

@gafter @AlekseyTs What do you think about my proposal for either GetConstantValueInfo().ConvertedValue or GetConvertedConstantValue() and making the constant value missing?

@jcouv jcouv self-assigned this Aug 8, 2019
@jcouv jcouv modified the milestones: Backlog, 16.4 Aug 8, 2019
@jcouv jcouv added 3 - Working 4 - In Review A fix for the issue is submitted for review. and removed 3 - Working labels Aug 8, 2019
@jcouv
Copy link
Member Author

jcouv commented Aug 20, 2019

I have a PR revising conversions on default literal. As part of that work, I tried changing the behavior of GetTypeInfo(...).Type for default, but saw that had significant impact on the IDE code. To minimize pain for API consumers, I'll keep the current public behavior (.Type returns the target-type instead of the null natural type), maintaining compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - In Review A fix for the issue is submitted for review. Area-Compilers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants