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.

Sign up for GitHub

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

Jump to bottom

Implement parsing for records #40467

Merged
merged 7 commits into from
Jan 5, 2020
Merged

Implement parsing for records #40467

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bb4d518
Make more space for DeclarationModifiers
agocke Dec 18, 2019
dc69d22
Implement parsing for the records
agocke Dec 6, 2019
e1b84b1
Respond to PR comments
agocke Jan 1, 2020
2d2d451
Respond to PR comments
agocke Jan 1, 2020
7f3601e
Update test baselines
agocke Jan 2, 2020
31b8fc5
Respond to PR comments
agocke Jan 2, 2020
196d8f5
Remove extra public API members
agocke Jan 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion eng/targets/Settings.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@
<DisableImplicitNuGetFallbackFolder>true</DisableImplicitNuGetFallbackFolder>
<ToolsetPackagesDir>$(RepoRoot)build\ToolsetPackages\</ToolsetPackagesDir>

<RoslynPortableTargetFrameworks>net472;netcoreapp2.1</RoslynPortableTargetFrameworks>
<!-- PROTOTYPE: Swap the order of the frameworks back before merging -->
<RoslynPortableTargetFrameworks>netcoreapp2.1;net472</RoslynPortableTargetFrameworks>
Copy link
Member

Choose a reason for hiding this comment

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

Please revert this change. Quoting from @jaredpar a few months ago:

We need to keep net472 as the first TF listed because that controls items like which tests run by default.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's the point. We should swap the default. I can't test on Mac using VS Code unless it's first.

Copy link
Member

Choose a reason for hiding this comment

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

We should swap the default. I can't test on Mac using VS Code unless it's first.

Definitely don't want us to do this. As @333fred noted, the order here is deliberate. Changing it will not fix anything, instead it will just fix one scenario and break another. In this case it would be fixing Mac, the minority scenario, and breaking Windows, the dominant scenario.

Where is the bug on VS Code that prevents Mac testing here? Have you filed a bug?

Copy link
Member Author

@agocke agocke Jan 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Why would it break Windows? The test explorer seems to work fine. Doesn't look like there's a bug here for VS Code. They don't support debugging on anything but .NET Core, and switching between target frameworks isn't a bug, it's a feature they don't have.

Copy link
Member

Choose a reason for hiding this comment

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

Why would it break Windows?

The set of tests run for desktop and core are significantly different. Moving to core by default will break the expectations of developers that expect to get the desktop tests.

If you think the trade off is a good one here then the correct course of action is to discuss the policy with the team. Not change it subtly in a PR. Particularly since this has come up several times before and we've kept the current order for these reasons.

Doesn't look like there's a bug here for VS Code.

VS Code cannot test code that every other tool in our toolbox can test. Is the explicit design of VS Code that they can't support multi-targeted projects? If so that seems really unfortunate.

it's a feature they don't have.

Where is the issue tracking the feature request?

Copy link
Member Author

Choose a reason for hiding this comment

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

If you think the trade off is a good one here then the correct course of action is to discuss the policy with the team. Not change it subtly in a PR. Particularly since this has come up several times before and we've kept the current order for these reasons.

Yup, this is just going into a feature branch. I had planned to send an email out before changing it in master. Can I leave a PROTOTYPE comment above and we can do that when I'm back in Seattle?

Where is the issue tracking the feature request?

One thing we have to change for that is running our desktop tests as 64-bit by default. For the life of me I cannot figure out how to change that from 32-bit. I gave up after a couple hours.

dotnet/vscode-csharp#1716

You aren't doing anything wrong. x86 debugging is not supported by the C# extension. You will need full Visual Studio for that.

Beyond that, I'm not sure what feature we would be asking for. Some way to change the framework the tests are run under? VS only got that a couple months ago, so it's not exactly something I'd put at the top of the VS Code priority list.

Copy link
Member

Choose a reason for hiding this comment

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

Can I leave a PROTOTYPE comment above and we can do that when I'm back in Seattle?

That's fine by me.

Beyond that, I'm not sure what feature we would be asking for. Some way to change the framework the tests are run under?

I think the ask is pretty straight forward: we have a project which is multi-targeted to desktop and .NET Core. When that happens don't fail, instead prefer the .NET Core target framework. Basically do the only action which is capable of succeeding in this environment.

<RoslynCheckCodeStyle Condition="'$(ContinuousIntegrationBuild)' != 'true' or '$(RoslynEnforceCodeStyle)' == 'true'">true</RoslynCheckCodeStyle>
<UseSharedCompilation>true</UseSharedCompilation>

Expand Down
3 changes: 3 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5927,4 +5927,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="ERR_ExternEventInitializer" xml:space="preserve">
<value>'{0}': extern event cannot have initializer</value>
</data>
<data name="IDS_FeatureRecords" xml:space="preserve">
<value>records</value>
</data>
</root>
16 changes: 13 additions & 3 deletions src/Compilers/CSharp/Portable/Declarations/DeclarationModifiers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Runtime.CompilerServices;

namespace Microsoft.CodeAnalysis.CSharp
{
[Flags]
internal enum DeclarationModifiers
internal enum DeclarationModifiers : uint
{
None = 0,
Abstract = 1 << 0,
Expand Down Expand Up @@ -34,9 +35,18 @@ internal enum DeclarationModifiers
Async = 1 << 20,
Ref = 1 << 21, // used only for structs

All = (1 << 22) - 1, // all modifiers
Unset = 1 << 22, // used when a modifiers value hasn't yet been computed
Data = 1 << 22,

All = (1 << 23) - 1, // all modifiers
Unset = 1 << 23, // used when a modifiers value hasn't yet been computed

AccessibilityMask = PrivateProtected | Private | Protected | Internal | ProtectedInternal | Public,
}

internal static class DeclarationModifierExtensions
{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static bool HasFlag(this DeclarationModifiers modifiers, DeclarationModifiers flag)
=> (modifiers & flag) != 0;
}
}
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/MessageID.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,7 @@ internal enum MessageID
IDS_FeatureSwitchExpression = MessageBase + 12763,
IDS_FeatureAsyncUsing = MessageBase + 12764,
IDS_FeatureLambdaDiscardParameters = MessageBase + 12765,
IDS_FeatureRecords = MessageBase + 12766,
}

// Message IDs may refer to strings that need to be localized.
Expand Down Expand Up @@ -293,6 +294,7 @@ internal static LanguageVersion RequiredVersion(this MessageID feature)
{
// Preview features.
case MessageID.IDS_FeatureLambdaDiscardParameters: // semantic check
case MessageID.IDS_FeatureRecords:
return LanguageVersion.Preview;

// C# 8.0 features.
Expand Down
5 changes: 3 additions & 2 deletions src/Compilers/CSharp/Portable/Generated/CSharp.Generated.g4
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

370 changes: 262 additions & 108 deletions src/Compilers/CSharp/Portable/Generated/Syntax.xml.Internal.Generated.cs
View file
Open in desktop

Large diffs are not rendered by default.

64 changes: 30 additions & 34 deletions src/Compilers/CSharp/Portable/Generated/Syntax.xml.Main.Generated.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading