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

Display 'readonly' for record structs #53634

Merged
merged 1 commit into from
May 23, 2021

Conversation

Youssef1313
Copy link
Member

Part of #53631.

@RikkiGibson
Copy link
Contributor

@jcouv for compiler sign-off

@dotnet/roslyn-ide for a small test sign-off

public async Task QuickInfoReadOnlyRecordStruct()
{
await TestWithOptionsAsync(
Options.Regular.WithLanguageVersion(LanguageVersion.CSharp9),
Copy link
Member Author

@Youssef1313 Youssef1313 May 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed I unintentionally left the language version from copy/pasting the test above. But the test should probably fail with C# 9.0

However, It's passing (CI test jobs has finished). Feels weird.


Edit: This is okay, the produced compilation has diagnostics as expected, but this doesn't prevent "correct" symbols from being created.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's expected. It should pass in C# 9. With some rare exceptions, we always bind in the latest version of the language, only producing different diagnostics. So the symbol is the same regardless of language version.

@@ -670,6 +669,15 @@ private void AddTypeKind(INamedTypeSymbol symbol)
break;

case TypeKind.Struct when symbol.IsRecord:
// In case ref record structs are allowed in future, call AddKeyword(SyntaxKind.RefKeyword) and remove assertion.
Copy link
Member

@jcouv jcouv May 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: The comment probably isn't needed

@@ -670,6 +669,15 @@ private void AddTypeKind(INamedTypeSymbol symbol)
break;

case TypeKind.Struct when symbol.IsRecord:
// In case ref record structs are allowed in future, call AddKeyword(SyntaxKind.RefKeyword) and remove assertion.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// In case ref record structs are allowed in future, call AddKeyword(SyntaxKind.RefKeyword) and remove assertion.

@jcouv jcouv merged commit 5cb928f into dotnet:main May 23, 2021
@ghost ghost added this to the Next milestone May 23, 2021
@jcouv
Copy link
Member

jcouv commented May 23, 2021

Merged/squashed. Thanks @Youssef1313

@Youssef1313 Youssef1313 deleted the readonly-record-struct-display branch May 23, 2021 17:40
@jcouv jcouv added New Language Feature - Records Records Community The pull request was submitted by a contributor who is not a Microsoft employee. labels May 23, 2021
333fred added a commit that referenced this pull request May 24, 2021
…ures/interpolated-string

* upstream/main: (92 commits)
  Keep casts when necessary to prefer a constant pattern over a type pattern
  Remove SyntaxKind.DataKeyword (#53614)
  Display 'readonly' for record structs (#53634)
  Update Building, Debugging, and Testing on Windows.md (#53543)
  Update dependencies from https://github.com/dotnet/arcade build 20210521.3 (#53617)
  Introduce resx for BuildValidator and MS.CA.Rebuild to allow localization (#53447)
  Report obsoletion diagnostics for slice and indexer (#53463)
  Update BasicGenerateConstructorDialog.cs
  Add searchbox in generate overrides dialog
  Allow `with` on anonymous types (#53248)
  Report diagnostic on correct node (#53538)
  Fix NotNullIfNotNull delegate conversion (#53409)
  Verify quick info session in InvokeQuickInfo
  Remove unnecessary retry
  Ensure no navbar IO on the UI thread
  Enable nullable reference types
  Fix timeout behavior in GetQuickInfo
  Add a semantic model based GetQuickInfoAsync entry point into QuickInfoServiceWithProviders
  Move semantic model based quick info API up to CommonQuickInfoProvider type
  Fix dnceng build by forcing the use of xcopy msbuild
  ...
@RikkiGibson RikkiGibson modified the milestones: Next, 17.0.P2 Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. New Language Feature - Records Records
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants