Skip to content

Display 'readonly' for record structs#53634

Merged
jcouv merged 1 commit intodotnet:mainfrom
Youssef1313:readonly-record-struct-display
May 23, 2021
Merged

Display 'readonly' for record structs#53634
jcouv merged 1 commit intodotnet:mainfrom
Youssef1313:readonly-record-struct-display

Conversation

@Youssef1313
Copy link
Member

Part of #53631.

@RikkiGibson
Copy link
Member

@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.

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

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 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. Feature - Records Records

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants