Skip to content

Conversation

@zhuman
Copy link
Contributor

@zhuman zhuman commented Aug 24, 2022

Closes #376

This change fixes support for C# language version 8.0 by swapping a ImplicitObjectCreationExpression (which uses the C# 9 language feature Type myField = new();) to instead use a normal ObjectCreationExpression with the type explicitly defined (Type myField = new Type();). This enables the source analyzer to work on .NET Core 3.0 projects once again, such as UWP projects.

As part of testing this fix, I found that the source generator tests as written today don't actually validate successful compilation of any supported scenario, and that a number of scenarios that the tests claim to work on C# 7.3 don't actually work anymore. Therefore I've also changed the following:

  • Added "unsupported C# language version" diagnostics to generators which produce C# 8.0 language features. This should be nicer for devs rather than just producing unsupported syntax and hitting random C# compiler errors. A future exercise could be to remove these features if it's desirable to support < C# 8.0.
  • For each of the UnsupportedCSharpLanguageVersion_* tests, I've also added validation that the source code does compile with C# 8.0.
  • I've changed all test source compilation to be done using C# 8.0.
  • I've added a dedicated source compilation success test case for ObservableProperty/ObservableObject.

PR Checklist

  • Created a feature/dev branch in your fork (vs. submitting directly from a commit on main)
  • Based off latest main branch of toolkit
  • PR doesn't include merge commits (always rebase on top of our main, if needed)
  • Tested code with current supported SDKs
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Contains NO breaking changes
  • Code follows all style conventions

Other information

@net-foundation-cla
Copy link

net-foundation-cla bot commented Aug 24, 2022

CLA assistant check
All CLA requirements met.

@zhuman
Copy link
Contributor Author

zhuman commented Aug 24, 2022

I discovered that the unit tests for the generators actually never check for compilation errors at all (it just checks for diagnostics coming directly from the generators, it doesn't check for errors in the generated code), so there are actually other cases where newer C# features are being generated and breaking tests. I am looking at how best to solve these.

Edit: see the description, I think the best route is to make these generators explicitly give the unsupported language version diagnostic and we should add tests that validate successful compilation. I've implemented these in my change.

zhuman added 2 commits August 23, 2022 19:09
…egisterAllGenerator to provide a nicer "unsupported C# language version" diagnostic instead of simply generating unsupported code and failing later in compilation
…t, update tests to use the minimum supported C# language version (8.0), and also validate that unsupported version cases actually succeed with the supported version
@zhuman zhuman marked this pull request as ready for review August 24, 2022 02:25
VariableDeclarator(Identifier(propertyName))
.WithInitializer(EqualsValueClause(
ImplicitObjectCreationExpression()
ObjectCreationExpression(IdentifierName(typeName))
Copy link
Contributor

Choose a reason for hiding this comment

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

We could guard it using language version but at this point, if we're going to drop .NET Native target as commented in the issue, then we could atleast warn about the minimum language version requirement.

I already did something like this in my private fork to generate language version compatible code. So, there's no need to check/warn about the version requirement. It always produces compilable code for the given target runtime.

.Select(static (item, _) => Execute.GetInfo(item))
.WithComparer(ValidationInfo.Comparer.Default);

context.FilterWithLanguageVersion(ref validationInfo, LanguageVersion.CSharp8, UnsupportedCSharpLanguageVersionError);
Copy link
Member

Choose a reason for hiding this comment

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

The ovservable validator doesn't use C# 8 features, no? This seems unnecessary (it's not free).

Copy link
Contributor

Choose a reason for hiding this comment

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

Even so, it's better to check against a min version of the language instead of leaving it up in the air!

Copy link
Member

@Sergio0694 Sergio0694 Sep 8, 2022

Choose a reason for hiding this comment

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

No, it isn't. As I said, this adds too much overhead. Besides, I already plan to move this out of the generator.
I'd really like for this PR to only fix that one thing so it's nice and simple and we can merge it quickly.

EDIT: on second thought, the generator should just skip, but never cause a build error here.

}";

// This is explicitly allowed in C# < 8.0, as it doesn't use any new features
VerifyGeneratedDiagnostics<ObservableValidatorValidateAllPropertiesGenerator>(
Copy link
Member

Choose a reason for hiding this comment

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

Undo this change (see previous comment), this generator doesn't require C# 8

new ObservableValidatorValidateAllPropertiesGenerator(),
new RelayCommandGenerator()
};
VerifyGeneratedDiagnostics(CSharpSyntaxTree.ParseText(source, CSharpParseOptions.Default.WithLanguageVersion(LanguageVersion.CSharp8)), generators, new string[] { });
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be updated so that only the lang version required by each generator is used, not just C# 8 for all.

These two generators should never cause a build error
Copy link
Member

@Sergio0694 Sergio0694 left a comment

Choose a reason for hiding this comment

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

Pushed a couple tweaks, LGTM! :shipit:

Thank you!

@Sergio0694 Sergio0694 merged commit 4fae4dd into CommunityToolkit:main Sep 8, 2022
@crramirez
Copy link

Hello,

One question. Have you tested with UWP projects? Because we are stuck on .Net Core 2.2 and C# 7.3, not .Net Core 3.0. We can use C# 8.0 in .net 2.2 but without all the features: https://www.reflectionit.nl/blog/2019/using-c-8-0-in-core-2-x-net-framework-and-uwp-projects

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Toolkit Doesn't Detect UnSupported C# Version or Doesn't work on C# 8

4 participants