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

[SG] Add support for Source Generator #2958

Merged
merged 256 commits into from
Nov 1, 2022
Merged

[SG] Add support for Source Generator #2958

merged 256 commits into from
Nov 1, 2022

Conversation

papafe
Copy link
Contributor

@papafe papafe commented Jun 13, 2022

No description provided.

# Conflicts:
#	Realm/Realm/DatabaseTypes/RealmObjectBase.cs
#	Realm/Realm/Dynamic/MetaRealmObject.cs
#	Realm/Realm/GlobalSuppressions.cs
#	Realm/Realm/Schema/ObjectSchema.cs
@@ -1985,7 +1985,7 @@
{
_realm.ThrowIfDisposed();

var query = (RealmResults<RealmObject>)All(className);
var query = (RealmResults<IRealmObject>)All(className);

Check warning

Code scanning / CodeQL

Cast from abstract to concrete collection

Questionable cast from abstract 'IQueryable<dynamic>' to concrete implementation 'RealmResults<IRealmObject>'.
Copy link
Member

@nirinchev nirinchev left a comment

Choose a reason for hiding this comment

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

I went through a portion of this and have mostly stylistic comments. A few general comments which didn't have a concrete line number:

  1. All the changes to Realm-Windows.sln need to also be applied to Realm.sln.
  2. There seem to still be some places where we can end up with awkward spacing - while not critical, it does trigger my OCD and I'd like to do our best to generate code that is nice to look at.


internal {_accessorInterfaceName} Accessor => _accessor = _accessor ?? new {_unmanagedAccessorClassName}(typeof({_classInfo.Name}));

[IgnoreDataMember, XmlIgnore]
Copy link
Member

Choose a reason for hiding this comment

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

We don't have XmlIgnore on many of these members in the RealmObjectBase class - should we have it or should we remove it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought that the reason why we added XmlIgnore only to some data members was that in some cases IgnoreDataMember was not enough. Given that adding it shouldn't be a problem, I've added it in all places in which we also have IgnoreDataMember

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think, is it fine to keep them?

Copy link
Member

@nirinchev nirinchev left a comment

Choose a reason for hiding this comment

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

I think this is almost ready to go. I have some code cleanup comments and a bunch of places where I've commented on the generated class where we need to more carefully handle corner cases to ensure we generate beautiful code.

: IRealmAccessor
// Should be used by generator and undocumented
[EditorBrowsable(EditorBrowsableState.Never)]
public abstract class UnmanagedAccessor : IRealmAccessor
Copy link
Member

Choose a reason for hiding this comment

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

Should we suppress this?

@papafe
Copy link
Contributor Author

papafe commented Oct 7, 2022

As a note, don't look at the CI. I've merged back into this branch only the changes I've done in the Realm folder, so they were easier to read here, so there's a lot of things missing that are in the other PR

@papafe papafe changed the title [WIP] Add base SG Add base SG Oct 11, 2022
@papafe papafe marked this pull request as ready for review October 11, 2022 08:47
@papafe papafe changed the title Add base SG [SG] Add base SG Oct 11, 2022
# Conflicts:
#	Realm/Realm/DatabaseTypes/Accessors/ManagedAccessor.cs
#	Realm/Realm/DatabaseTypes/Accessors/UnmanagedAccessor.cs
#	Tests/Realm.Tests/Sync/AsymmetricObjectTests.cs
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

return;
}

if (classSymbol.IsAnyRealmObjectType())

Check warning

Code scanning / CodeQL

Dereferenced variable may be null

Variable [classSymbol](1) may be null at this access because of [this](2) assignment.
{
internal class DiagnosticsEmitter
{
private GeneratorExecutionContext _context;

Check notice

Code scanning / CodeQL

Missed 'readonly' opportunity

Field '_context' can be 'readonly'.
{
internal class Parser
{
private GeneratorExecutionContext _context;

Check notice

Code scanning / CodeQL

Missed 'readonly' opportunity

Field '_context' can be 'readonly'.
{
internal static class Utils
{
private static List<SpecialType> _validRealmIntegerArgumentTypes = new()

Check notice

Code scanning / CodeQL

Missed 'readonly' opportunity

Field '_validRealmIntegerArgumentTypes' can be 'readonly'.
SpecialType.System_Int64
};

private static List<SpecialType> _validIntegerTypes = new()

Check notice

Code scanning / CodeQL

Missed 'readonly' opportunity

Field '_validIntegerTypes' can be 'readonly'.
Comment on lines +624 to +636
if (setValueLines.Length == 0)
{
setValueBody = $@"throw new MissingMemberException($""The object does not have a settable Realm property with name {{propertyName}}"");";
}
else
{
setValueBody = $@"switch (propertyName)
{{
{setValueLines.Indent(1, trimNewLines: true)}
default:
throw new MissingMemberException($""The object does not have a settable Realm property with name {{propertyName}}"");
}}";
}

Check notice

Code scanning / CodeQL

Missed ternary opportunity

Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
Comment on lines +647 to +658
if (getListValueLines.Length == 0)
{
getListValueBody = $@"throw new MissingMemberException($""The object does not have a Realm list property with name {{propertyName}}"");";
}
else
{
getListValueBody = $@"return propertyName switch
{{
{getListValueLines}
_ => throw new MissingMemberException($""The object does not have a Realm list property with name {{propertyName}}""),
}};";
}

Check notice

Code scanning / CodeQL

Missed ternary opportunity

Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
Comment on lines +663 to +674
if (getSetValueLines.Length == 0)
{
getSetValueBody = $@"throw new MissingMemberException($""The object does not have a Realm set property with name {{propertyName}}"");";
}
else
{
getSetValueBody = $@"return propertyName switch
{{
{getSetValueLines}
_ => throw new MissingMemberException($""The object does not have a Realm set property with name {{propertyName}}""),
}};";
}

Check notice

Code scanning / CodeQL

Missed ternary opportunity

Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
Comment on lines +679 to +690
if (getDictionaryValueLines.Length == 0)
{
getDictionaryValueBody = $@"throw new MissingMemberException($""The object does not have a Realm dictionary property with name {{propertyName}}"");";
}
else
{
getDictionaryValueBody = $@"return propertyName switch
{{
{getDictionaryValueLines.Indent(1, trimNewLines: true)}
_ => throw new MissingMemberException($""The object does not have a Realm dictionary property with name {{propertyName}}""),
}};";
}

Check notice

Code scanning / CodeQL

Missed ternary opportunity

Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
Comment on lines +750 to +763
if (property.TypeInfo.IsBacklink)
{
getFieldString = "GetBacklinks";
}
else
{
getFieldString = property.TypeInfo.CollectionType switch
{
CollectionType.List => "GetListValue",
CollectionType.Set => "GetSetValue",
CollectionType.Dictionary => "GetDictionaryValue",
_ => throw new NotImplementedException(),
};
}

Check notice

Code scanning / CodeQL

Missed ternary opportunity

Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
# Conflicts:
#	.github/templates/common.lib.yml
#	.github/templates/test.lib.yml
#	.github/workflows/test-android.yml
#	.github/workflows/test-ios.yml
#	.github/workflows/test-macos.yml
#	.github/workflows/test-net-core.yml
#	.github/workflows/test-net-framework.yml
#	.github/workflows/test-uwp-managed.yml
@papafe papafe changed the title [SG] Add base SG [SG] Add support for Source Generator Nov 1, 2022
@papafe papafe merged commit a7b8f3a into main Nov 1, 2022
@papafe papafe deleted the fp/sg-scaffolding branch November 1, 2022 14:48
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants