Improved nullable support #30

jkurdek · 2024-05-08T13:29:13Z

No description provided.

src/Controls/tests/BindingSourceGen.UnitTests/IntegrationTests.cs

simonrozsival

I like the end-to-end tests that you added. I'm not sure if we should transfor the path as it's parsed or if we should just modify the code that produces the C# code. I'm leaning towards the latter, it seems less magic. The code writer should just have some a parameter like bool considerAllReferenceTypesPotentiallyNullable and generate a nullability check in the setter for each reference type and always add ?s after accessing a reference type when constructing the partial getters for the handlers array.

src/Controls/src/BindingSourceGen/GeneratorDataModels.cs

src/Controls/src/BindingSourceGen/PathParser.cs

simonrozsival · 2024-05-10T12:32:52Z

src/Controls/src/BindingSourceGen/SetterBuilder.cs

+                else if (previousPart is MemberAccess memberAccess && !memberAccess.IsMemberValueType && _considerAllReferenceTypesPotentiallyNullable) 
+                {
+                    AddIsExpression("{}");
+                    _expression = AccessExpressionBuilder.Build(_expression, memberAccess);
+                }


I don't think this is equivalent to the removed condition in Setter.From. If this didn't break any existing test though, I need to think a bit if we are missing a test scenario or not.

This is still WIP. The previous version contained a bug. When we had nullable source and first access in the path was conditionalAccess it generated if p0 is {} p1.

oh, good catch!

jkurdek · 2024-05-10T15:27:56Z

TODO:

Indexer support
Review tests
Add additional tests to SetterBuilderTests
Refactor

simonrozsival · 2024-05-13T12:43:27Z

src/Controls/src/BindingSourceGen/SetterBuilder.cs

        public void AddPart(IPathPart nextPart)
        {
-            _previousPart = HandlePreviousPart(nextPart);
+            var newPart = HandleCurrentPart(nextPart);
+            _previousPart = _currentPart;
+            _currentPart = newPart;
+
        }


Thinking out loud: this needs refactoring, the nextPart, newPart, _currentPart, _previousPart naming is confusing.

simonrozsival · 2024-05-13T12:46:51Z

src/Controls/src/BindingSourceGen/PathParser.cs

+        var isReferenceType = typeSymbol?.IsReferenceType ?? false;
+        var isNullableValueType = typeSymbol != null ? BindingGenerationUtilities.IsNullableValueType(typeSymbol) : false;


There's quite a lot of code duplication going on here and it's getting hard to read. We should consider moving this to a separate method at least.

simonrozsival · 2024-05-13T12:52:29Z

src/Controls/src/BindingSourceGen/BindingCodeWriter.cs

 			foreach (var part in path)
 			{
 				var previousExpression = nextExpression;
 				nextExpression = AccessExpressionBuilder.Build(previousExpression, MaybeWrapInConditionalAccess(part, forceConditonalAccessToNextPart));
-				forceConditonalAccessToNextPart = part is Cast;
+				var isNullableReferenceType = part is MemberAccess memberAccess && !memberAccess.IsValueType;


We need a check for IndexAccess here as well + a unit test that covers that case.

Co-authored-by: Šimon Rozsíval <simon@rozsival.com>

* added unit tests for nullable disabled * added integration tests for nullable disabled * add nullable disabled support for reference types * wip: added support for nullable disabled on value types * improved integration tests * initialize support for indexers * add indexers support to SetterBuilder * added additional tests for setter builder * cleaned up SetterBuilder * Fixed nullable access with BindingTransformer Co-authored-by: Šimon Rozsíval <simon@rozsival.com> * Removed IsNullableValueType property Co-authored-by: Šimon Rozsíval <simon@rozsival.com> * Cleaned up nullability Co-authored-by: Šimon Rozsíval <simon@rozsival.com> --------- Co-authored-by: Šimon Rozsíval <simon@rozsival.com>

* Add new public API SetBinding<TSource, TProperty> * Add source generator skeleton WIP * Setup unit tests for binding intermediate representation * Added basic nullability support * Remove unnecessary Id from the binding record * Generate casts * Split index and member access * Cleanup * Update test case * Cleanup * Fix the semantic of conditional access * add as-cast suport to source generator * improve nullability check * specify params in tests only when not default * simplify diagnostics * move path parser to separate class * small cleanup * Get nullability right in binding representation tests Co-authored-by: Šimon Rozsíval <simon@rozsival.com> * Fix path parse to work with improved tests * Integration tests (#14) * create assert extensions Co-authored-by: Šimon Rozsíval <simon@rozsival.com> * added basic binding integration test * fixed unit tests Co-authored-by: Šimon Rozsíval <simon@rozsival.com> * added cast integration test Co-authored-by: Šimon Rozsíval <simon@rozsival.com> * cleaned up tests * remove nullableEnabled param from PathParser * more clean up * Enable diagnostic generation in PathParser * refactored parse path function * further parsePath refactoring * refactored source generator utilities * fixed lambda return type nullability inference * replaced linkedList with list in ParsePath --------- Co-authored-by: Šimon Rozsíval <simon@rozsival.com> * Implement setters (#16) * Make Cast record non-nested * Simplify AccessExpressionBuilder * Add SetterBuilder * Fix BindingCodeWriter * Fix tests * Make SetterBuilder private inside Setter * Remove unnecessary IsConditional * Simplify indexes (#18) * Fix overload check (#20) * fixed correct overload check * disable test * Implement detection of writable properties (#19) * Add TODOs to change arrays to equatable arrays * Add projects to solutions * Try to run unit tests in CI * Added custom indexer support * Fix typo * Avoid allocating line separators array * Hide the new SetBinding overload from editor suggestions for now * Incremental generation tests * replaced array with equatable array * Added source information + formatting * added third party licenses * Add benchmark for source-generated SetBinding (#25) * Add benchmark for source-generated SetBinding * Auto-format source code * improve overload diagnostics tests * added more complex overload tests * improve predicate method filtering * improved diagnostics on overload detection * Auto-format source code * add ArgumentOutOfRangeException * Auto-format source code --------- Co-authored-by: GitHub Actions Autoformatter <autoformat@example.com> Co-authored-by: Jeremi Kurdek <jkurdek@gmail.com> * Improve diagnostic messages * Improved incrementality testing (#28) * improved incrementality testing * added source changed test * added different file modified test * Add C-Style casts support (#26) * initialized cstyle casts * added integration and accessExpressionBuilder tests * added explicit cast to as cast adapter * Simplify inserting conditional access * Add integration tests --------- Co-authored-by: Simon Rozsival <simon@rozsival.com> * Cleanup (#29) * Improved nullable support (#30) * added unit tests for nullable disabled * added integration tests for nullable disabled * add nullable disabled support for reference types * wip: added support for nullable disabled on value types * improved integration tests * initialize support for indexers * add indexers support to SetterBuilder * added additional tests for setter builder * cleaned up SetterBuilder * Fixed nullable access with BindingTransformer Co-authored-by: Šimon Rozsíval <simon@rozsival.com> * Removed IsNullableValueType property Co-authored-by: Šimon Rozsíval <simon@rozsival.com> * Cleaned up nullability Co-authored-by: Šimon Rozsíval <simon@rozsival.com> --------- Co-authored-by: Šimon Rozsíval <simon@rozsival.com> * Simplify building setter * cleaned up methods in BindingSourceGeneratorUtilities * replaced tuples with Result<T> * simplified naming * Fix and improve unit test project * Fix bad conflict resolution in solution file --------- Co-authored-by: Jeremi Kurdek <jkurdek@gmail.com> Co-authored-by: Jeremi Kurdek <59935235+jkurdek@users.noreply.github.com> Co-authored-by: GitHub Actions Autoformatter <autoformat@example.com>

added unit tests for nullable disabled

a4d737c

jkurdek changed the title ~~added unit tests for nullable disabled~~ Improved nullable support May 8, 2024

added integration tests for nullable disabled

88dfdcf

jkurdek requested a review from simonrozsival May 8, 2024 13:53

simonrozsival reviewed May 9, 2024

View reviewed changes

src/Controls/tests/BindingSourceGen.UnitTests/IntegrationTests.cs Outdated Show resolved Hide resolved

simonrozsival reviewed May 9, 2024

View reviewed changes

src/Controls/tests/BindingSourceGen.UnitTests/IntegrationTests.cs Outdated Show resolved Hide resolved

simonrozsival reviewed May 9, 2024

View reviewed changes

add nullable disabled support for reference types

c5bc172

simonrozsival reviewed May 10, 2024

View reviewed changes

src/Controls/src/BindingSourceGen/GeneratorDataModels.cs Show resolved Hide resolved

simonrozsival reviewed May 10, 2024

View reviewed changes

src/Controls/src/BindingSourceGen/PathParser.cs Outdated Show resolved Hide resolved

simonrozsival reviewed May 10, 2024

View reviewed changes

src/Controls/src/BindingSourceGen/PathParser.cs Outdated Show resolved Hide resolved

simonrozsival reviewed May 10, 2024

View reviewed changes

jkurdek added 2 commits May 10, 2024 16:58

wip: added support for nullable disabled on value types

0d06066

improved integration tests

1116173

jkurdek added 4 commits May 10, 2024 18:14

initialize support for indexers

cd1f6fc

add indexers support to SetterBuilder

e85fede

added additional tests for setter builder

8c856ec

cleaned up SetterBuilder

86521ee

jkurdek marked this pull request as ready for review May 13, 2024 08:56

simonrozsival reviewed May 13, 2024

View reviewed changes

jkurdek and others added 3 commits May 14, 2024 15:41

Fixed nullable access with BindingTransformer

5cfce1f

Co-authored-by: Šimon Rozsíval <simon@rozsival.com>

Removed IsNullableValueType property

02b537a

Co-authored-by: Šimon Rozsíval <simon@rozsival.com>

Cleaned up nullability

2c80122

Co-authored-by: Šimon Rozsíval <simon@rozsival.com>

simonrozsival approved these changes May 14, 2024

View reviewed changes

simonrozsival merged commit bff3429 into simonrozsival:binding-source-generator May 14, 2024
2 checks passed

github-actions bot locked and limited conversation to collaborators Jun 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved nullable support #30

Improved nullable support #30

jkurdek commented May 8, 2024

simonrozsival left a comment

simonrozsival May 10, 2024

jkurdek May 10, 2024

simonrozsival May 10, 2024

jkurdek commented May 10, 2024

simonrozsival May 13, 2024

simonrozsival May 13, 2024

simonrozsival May 13, 2024

		var isReferenceType = typeSymbol?.IsReferenceType ?? false;
		var isNullableValueType = typeSymbol != null ? BindingGenerationUtilities.IsNullableValueType(typeSymbol) : false;

Improved nullable support #30

Improved nullable support #30

Conversation

jkurdek commented May 8, 2024

simonrozsival left a comment

Choose a reason for hiding this comment

simonrozsival May 10, 2024

Choose a reason for hiding this comment

jkurdek May 10, 2024

Choose a reason for hiding this comment

simonrozsival May 10, 2024

Choose a reason for hiding this comment

jkurdek commented May 10, 2024

simonrozsival May 13, 2024

Choose a reason for hiding this comment

simonrozsival May 13, 2024

Choose a reason for hiding this comment

simonrozsival May 13, 2024

Choose a reason for hiding this comment