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

Support declaring static fields, auto-properties and field-like events within interfaces. #20165

Conversation

AlekseyTs
Copy link
Contributor

@dotnet/roslyn-compiler Please review.

@AlekseyTs
Copy link
Contributor Author

https://ci.dot.net/job/dotnet_roslyn/job/features_DefaultInterfaceImplementation/job/windows_debug_unit64_prtest/34/

One or more test assemblies failed to run

One or more test assemblies did not produce results, possible causes: 1) The build was aborted out due to a long running or deadlocked test. 2) The test run for this assembly failed due to xUnit crashing (possibly due to a unhandled exception on a background thread or a stack overflow). 3) The test caused an assert dialog to be opened, which blocked the run. See the Console Log for more information. [xUnit] [ERROR] - The result file 'D:\j\workspace\windows_debug---fbd734f1\Binaries\Debug\UnitTests\CSharpEditorServicesTest\xUnitResults\Roslyn.Services.Editor.CSharp.UnitTests.dll.2.xml' for the metric 'xUnit.Net' is empty. The result file has been skipped.
Indication 1

Build Timed Out or was Manually Aborted

The build timed out, or was forcibly aborted after reaching the maximum length. View the log right before the flagged timeout message to see what was happening. Build timed out (after 120 minutes). Marking the build as aborted.
Indication 2 Indication 3

There are errors when processing test results

There was an error processing the xunit test results: [xUnit] [ERROR] - The result file 'D:\j\workspace\windows_debug---fbd734f1\Binaries\Debug\UnitTests\CSharpEditorServicesTest\xUnitResults\Roslyn.Services.Editor.CSharp.UnitTests.dll.2.xml' for the metric 'xUnit.Net' is empty. The result file has been skipped.
Indication 4

@AlekseyTs
Copy link
Contributor Author

Test windows_debug_unit64_prtest please

@AlekseyTs
Copy link
Contributor Author

https://ci.dot.net/job/dotnet_roslyn/job/features_DefaultInterfaceImplementation/job/windows_release_vs-integration_prtest/34/

CLR test failure (only matches the first, see "Latest Test Result" link for the rest)

Roslyn.VisualStudio.IntegrationTests.VisualBasic.BasicExpressionEvaluator.StateMachineTypeParameters [FAIL]
Indication 1

MSBuild Error

error MSB3073: The command ""Binaries\Release\Exes\RunTests\RunTests.exe" C:\Users\dotnet-bot/.nuget/packages/\xunit.runner.console\2.2.0-beta4-build3444\tools -xml -testVsi -log:"D:\j\workspace\windows_relea---03c9fade\Binaries\Release\runtests.log" D:\j\workspace\windows_relea---03c9fade\Binaries\Release\UnitTests\VisualStudioIntegrationTests\Roslyn.VisualStudio.IntegrationTests.dll" exited with code 1.
Indication 2

@AlekseyTs
Copy link
Contributor Author

Test windows_release_vs-integration_prtest please

@AlekseyTs
Copy link
Contributor Author

@dotnet/roslyn-compiler Please review.

{
// PROTOTYPE(DefaultInterfaceImplementation): It looks like some flavor of static members was supported by
Copy link
Member

Choose a reason for hiding this comment

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

Unless we're confident static members worked in all runtimes, maybe it's safer to rely on explicit declaration of support by the runtime, even for static members?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a part of the current runtime spec. @gafter also thought this would be the right thing to do (#18519 (review)).

@@ -101,7 +101,7 @@ internal sealed class SourcePropertySymbol : PropertySymbol, IAttributeTargetSym

this.CheckModifiers(location, isIndexer, diagnostics);

isAutoProperty = isAutoProperty && (!containingType.IsInterface && !IsAbstract && !IsExtern && !isIndexer && hasAccessorList);
isAutoProperty = isAutoProperty && (!(containingType.IsInterface && !IsStatic) && !IsAbstract && !IsExtern && !isIndexer && hasAccessorList);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe remove the outer parens?

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM

@AlekseyTs
Copy link
Contributor Author

@dotnet/roslyn-compiler Please review, need a second sign-off.

@@ -1803,6 +1803,8 @@ private static SyntaxToken GetImplicitConstructorBodyToken(CSharpSyntaxNode cont
{
case SyntaxKind.ClassDeclaration:
return ((ClassDeclarationSyntax)containerNode).OpenBraceToken;
case SyntaxKind.InterfaceDeclaration:
return ((InterfaceDeclarationSyntax)containerNode).OpenBraceToken;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe collapse all the cases and cast to BaseTypeDeclarationSyntax to get the OpenBrace ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, I'll make this change in the next PR.

@@ -1912,8 +1912,8 @@ public interface I1
var compilation1 = CreateStandardCompilation(source1, options: TestOptions.DebugDll,
parseOptions: TestOptions.Regular.WithLanguageVersion(LanguageVersion.Latest));
Assert.True(compilation1.Assembly.RuntimeSupportsDefaultInterfaceImplementation);
compilation1.VerifyDiagnostics(
// (4,9): error CS8052: Auto-implemented properties inside interfaces cannot have initializers.
compilation1.VerifyEmitDiagnostics(
Copy link
Member

@VSadov VSadov Jun 13, 2017

Choose a reason for hiding this comment

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

VerifyEmitDiagnostics [](start = 25, length = 21)

Why Emit diagnostics? Is seems these errors are still detected at binding. #Resolved

Copy link
Contributor Author

@AlekseyTs AlekseyTs Jun 13, 2017

Choose a reason for hiding this comment

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

I don't want emit to crash or assert in this situation #Resolved

Copy link
Member

Choose a reason for hiding this comment

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

It seems like we want to preserve the part where these errors are detected at binding. <aybe make just some of them use VerifyEmitDiagnostics.
I think the possibility of trying to emit in the presence of errors is a more general problem.


In reply to: 121768940 [](ancestors = 121768940)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like we want to preserve the part where these errors are detected at binding.
We do have a test like that.


In reply to: 121770162 [](ancestors = 121770162,121768940)

Copy link
Member

@VSadov VSadov left a comment

Choose a reason for hiding this comment

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

LGTM

@AlekseyTs AlekseyTs merged commit b2a061e into dotnet:features/DefaultInterfaceImplementation Jun 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants