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

c# 7.1 feature `default` is not correctly handled #1982

Closed
jskeet opened this Issue Aug 19, 2017 · 6 comments

Comments

Projects
None yet
3 participants
@jskeet

jskeet commented Aug 19, 2017

DocFX Version Used: 22.2.3

(And .NET Core SDK 2.0, on Windows.)

Template used: default

Steps to Reproduce:

Project file:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <TargetFramework>netstandard1.4</TargetFramework>
    <LangVersion>latest</LangVersion>
  </PropertyGroup>
</Project>

docfx.json:

{
  "metadata": [{
    "src": [{ "files": ["Project.csproj"] }],
    "dest": "api",
  }]
}

Sole class (Class1.cs):

using System.Threading.Tasks;

namespace Project
{
    public class Class1
    {
        /// <param name="x">A parameter</param>
        public Task Foo(int x = default) => (Task) null;
    }
}

Expected Behavior:

Running docfx metadata -f should not cause any errors.

Actual Behavior:

The following error is generated:

Error:Error extracting metadata for c:/Users/jon/Test/Projects/docfxbug/Project/Project.csproj: Value cannot be null.
Parameter name: name

Note that changing the code even slightly removes the error:

  • Using default(int) instead of default
  • Giving the method a regular body
  • Removing the XML documentation for the parameter
  • Changing the return value to just null instead of the cast

No idea what's going on or what's generating the error... I'm hoping this is sufficiently self-contained that it should be easy for the team to diagnose.

vicancy added a commit that referenced this issue Aug 21, 2017

@vicancy vicancy closed this in 75c2605 Aug 22, 2017

vicancy added a commit that referenced this issue Aug 22, 2017

@vicancy

This comment has been minimized.

Show comment
Hide comment
@vicancy

vicancy Aug 22, 2017

Contributor

Fixed in 2.23.1

Contributor

vicancy commented Aug 22, 2017

Fixed in 2.23.1

@jskeet

This comment has been minimized.

Show comment
Hide comment
@jskeet

jskeet Aug 22, 2017

Firstly, thanks for jumping on this so quickly - it's much appreciated.

While this now produced metadata without an error, it's not producing the right metadata.

Generating the API from my sample code above using 2.23.1, I get a child with a UID of Project.Class1.Foo(System.Int32,System.), and a declaration syntax of:

public Task Foo(int x = null, )

The trailing comma is incorrect, as is the null. (It should be 0.) I'd expect the output from docfx to be exactly the same, regardless of whether I use = default or = default(int) in the parameter declaration.

In my real work projects, I'm seeing lots of members just not being generated at all, which is very odd. I haven't been able to reproduce that in a minimal test project yet, but please let me know if you'd like me to investigate that aspect further.

jskeet commented Aug 22, 2017

Firstly, thanks for jumping on this so quickly - it's much appreciated.

While this now produced metadata without an error, it's not producing the right metadata.

Generating the API from my sample code above using 2.23.1, I get a child with a UID of Project.Class1.Foo(System.Int32,System.), and a declaration syntax of:

public Task Foo(int x = null, )

The trailing comma is incorrect, as is the null. (It should be 0.) I'd expect the output from docfx to be exactly the same, regardless of whether I use = default or = default(int) in the parameter declaration.

In my real work projects, I'm seeing lots of members just not being generated at all, which is very odd. I haven't been able to reproduce that in a minimal test project yet, but please let me know if you'd like me to investigate that aspect further.

@vicancy

This comment has been minimized.

Show comment
Hide comment
@vicancy

vicancy Aug 22, 2017

Contributor

Hi @jskeet as for * I'm seeing lots of members just not being generated at all*, is it specifically for c# 7.1 features or a general bug?

Contributor

vicancy commented Aug 22, 2017

Hi @jskeet as for * I'm seeing lots of members just not being generated at all*, is it specifically for c# 7.1 features or a general bug?

@vicancy vicancy changed the title from "Value cannot be null" error when using default literal to c# 7.1 feature `default` is not correctly handled Aug 22, 2017

@vicancy vicancy reopened this Aug 22, 2017

@jskeet

This comment has been minimized.

Show comment
Hide comment
@jskeet

jskeet Aug 22, 2017

It's specifically when some members in the same file (not necessarily those that aren't being generated) use the new default literal syntax.

Using 2.23.1 on the Google.Cloud.Translation.V2 project in https://github.com/GoogleCloudPlatform/google-cloud-dotnet, I get:

  • 19 members in Google.Cloud.Translation.V2.TranslationClient.yml when building from master
  • 10 members when building with PR 1388

jskeet commented Aug 22, 2017

It's specifically when some members in the same file (not necessarily those that aren't being generated) use the new default literal syntax.

Using 2.23.1 on the Google.Cloud.Translation.V2 project in https://github.com/GoogleCloudPlatform/google-cloud-dotnet, I get:

  • 19 members in Google.Cloud.Translation.V2.TranslationClient.yml when building from master
  • 10 members when building with PR 1388
@jskeet

This comment has been minimized.

Show comment
Hide comment
@jskeet

jskeet Aug 22, 2017

That's a pretty large repo though, and I wouldn't expect you to pull it and get it all sorted - if there's no obvious reason why this would be taking place, I'm happy to try to reproduce in a small project. Would you prefer me to start a new issue for that, or keep it in here? (It may well be related but not the same...)

jskeet commented Aug 22, 2017

That's a pretty large repo though, and I wouldn't expect you to pull it and get it all sorted - if there's no obvious reason why this would be taking place, I'm happy to try to reproduce in a small project. Would you prefer me to start a new issue for that, or keep it in here? (It may well be related but not the same...)

@vicancy

This comment has been minimized.

Show comment
Hide comment
@vicancy

vicancy Aug 22, 2017

Contributor

Thanks for reporting the issue. I will take a look first and let you know if any further help is needed.

Contributor

vicancy commented Aug 22, 2017

Thanks for reporting the issue. I will take a look first and let you know if any further help is needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment