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

Handle file-scoped namespaces in Logging Source Generator #57894

Merged

Conversation

martincostello
Copy link
Member

Handle namespace not being emitted when file-scoped namespaces are used for a log class.

Relates to #57880.

Handle namespace not being emitted when file-scoped
namespaces are used for a log class.
Relates to dotnet#57880.
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 22, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-Extensions-Logging and removed community-contribution Indicates that the PR has been added by a community member labels Aug 22, 2021
@ghost
Copy link

ghost commented Aug 22, 2021

Tagging subscribers to this area: @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

Handle namespace not being emitted when file-scoped namespaces are used for a log class.

Relates to #57880.

Author: martincostello
Assignees: -
Labels:

area-Extensions-Logging

Milestone: -

eng/Versions.props Outdated Show resolved Hide resolved
@eerhardt
Copy link
Member

Now that #57088 has been merged into main, this PR should be able to proceed.

@maryamariyan
Copy link
Member

Now that #57088 has been merged into main, this PR should be able to proceed.

@chsienki is there a more general way we could use to find the namespace (via the semantic model) rather than using BaseNamespaceDeclarationSyntax as recommended in #57880 (comment)?

@eerhardt
Copy link
Member

Also note we should test (and possibly fix) the System.Text.Json source generator for the same scenario.

@maryamariyan maryamariyan added this to the 6.0.0 milestone Sep 2, 2021
@martincostello
Copy link
Member Author

Anything more I need to do with this PR for merge?

@maryamariyan
Copy link
Member

confirmed with @sharwell offline that using BaseNamespaceDeclarationSyntax is fine here.

@maryamariyan
Copy link
Member

@martincostello would you be interested in taking a look at the Json source generator side as well for file-scoped namespaces either in this or in a separate PR?

@maryamariyan maryamariyan merged commit 73c0f8d into dotnet:main Sep 10, 2021
@danmoseley
Copy link
Member

Does this need porting? Would we otherwise be broken as soon as someone enables "automatic usings"?

@martincostello
Copy link
Member Author

@martincostello would you be interested in taking a look at the Json source generator side as well for file-scoped namespaces either in this or in a separate PR?

Sure, I'll take a look at some point this weekend.

@martincostello martincostello deleted the Fix-Logging-SourceGen-FileScoped-NS branch September 11, 2021 06:43
@martincostello
Copy link
Member Author

Does this need porting? Would we otherwise be broken as soon as someone enables "automatic usings"?

Yeah I think it would make sense to port this to the 6.0 branch, if someone with permissions could do the honours?

@martincostello
Copy link
Member Author

@maryamariyan Just created a quick console app to check out the JSON stuff with file-scoped namespaces and seems fine using version 6.0.100-rc.1.21453.49 of the SDK.

> dotnet build
Microsoft (R) Build Engine version 17.0.0-preview-21453-03+74e9935a4 for .NET
Copyright (C) Microsoft Corporation. All rights reserved.

  Determining projects to restore...
  All projects are up-to-date for restore.
  You are using a preview version of .NET. See: https://aka.ms/dotnet-core-preview
  JsonSourceGenerator -> C:\Coding\martincostello\JsonSourceGenerator\bin\Debug\net6.0\JsonSourceGenerator.dll

Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:00:00.86
> dotnet run
{"FirstName":"Jane","LastName":"Doe"}

JsonSourceGenerator.csproj

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
    <OutputType>Exe</OutputType>
    <RootNamespace>JsonSourceGenerator</RootNamespace>
    <TargetFramework>net6.0</TargetFramework>
  </PropertyGroup>
</Project>

MyJsonContext.cs

using System.Text.Json.Serialization;

namespace JsonSourceGenerator;

[JsonSerializable(typeof(Person))]
internal partial class MyJsonContext : JsonSerializerContext
{
}

Person.cs

namespace JsonSourceGenerator;

internal class Person
{
    public string? FirstName { get; set; }
    public string? LastName { get; set; }
}

Program.cs

using System.Text.Json;
using JsonSourceGenerator;

Person person = new() { FirstName = "Jane", LastName = "Doe" };

byte[] utf8Json = JsonSerializer.SerializeToUtf8Bytes(person, MyJsonContext.Default.Person);

person = JsonSerializer.Deserialize(utf8Json, MyJsonContext.Default.Person)!;

string json = JsonSerializer.Serialize(person, MyJsonContext.Default.Person);

Console.WriteLine(json);

@eerhardt
Copy link
Member

The reason Json works is because it is using ITypeSymbol.ContainingNamespace, which does the work.

public override string Namespace =>
IsArray ?
GetElementType().Namespace :
_typeSymbol.ContainingNamespace?.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat.WithGlobalNamespaceStyle(SymbolDisplayGlobalNamespaceStyle.OmittedAsContaining))!;

Maybe we should consider doing something similar in the Logging generator?

@maryamariyan
Copy link
Member

Maybe we should consider doing something similar in the Logging generator?

will take a look

maryamariyan pushed a commit to maryamariyan/runtime that referenced this pull request Sep 14, 2021
Handle namespace not being emitted when file-scoped
namespaces are used for a log class.
Relates to dotnet#57880.
@danmoseley
Copy link
Member

Does JSON need a test to protect this scenario?

ericstj pushed a commit that referenced this pull request Sep 17, 2021
…spaces (#57894) (#59100)

* Handle file-scoped namespaces (#57894)

Handle namespace not being emitted when file-scoped
namespaces are used for a log class.
Relates to #57880.

* Fix build issue

Co-authored-by: Martin Costello <martin@martincostello.com>
@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants