[discussion] Various suggestions

[Athari]

I'd like to hear your opinion on the following points. If you approve the suggestions, maybe I'll implement some of them.

1. Read-only attribute from .NET

There's a universally available System.ComponentModel.ReadOnlyAttribute, so there's no reason to create a separate ReadonlyAttribute. It's a bit convoluted as it requires an argument and can be set to IsReadOnly = false, but it has the bonus of being a part of .NET.

2. Automatic read-only

Whether a field is read-only should probably be detected from the AvaloniaProperty.RegisterDirect call — whether it has setter parameter or not.

3. Backing field attribute ergonomics

BackingFieldAttribute can have multiple constructors to cover all possible options:

• BackingFieldAttribute()
• BackingFieldAttribute(Accessibility)
• BackingFieldAttribute(string?, Accessibility?)

4. Inheriting from AvaloniaObject

I'm not 100% sure, but I think attached properties can be owned by non-AvaloniaObjects. Currently generator ignores these types.

5. Non-partial types

The generator should produce a warning when the type isn't partial. Though causing a compilation error like now is a valid option too. IDE even suggests adding the partial keyword.

6. Multiple types within one source file

The generator crashes when one source file contains multiple classes which require generation. It attempts to create two files with the same name and fails. I suspect the same issue happens when two source files happen to have the same name.

7. Incremental generator

Making the generator incremental would be nice for performance reasons.

8. Names of attached properties

Attached properties can't rely on nameof operator and have to pass a string as a name. Maybe it makes sense to generate constants like NameOfFooProperty or FooPropertyName to help with that.

[jp2masa]

Sorry for taking so long to reply. I'm currently not very active on GitHub.

Your suggestions are very interesting and some are even ideas I had back when I created the project. This is my opinion:

1. System.ComponentModel attributes are kind of special (used for serialization and other stuff, I think?), so I prefer not to mess with them.

2. I think I have some code for that, but I never published because it was not very stable IIRC. Maybe I will just wrap it in a try/catch. Sounds good?

3. I'm not sure why I created mutable properties instead of a constructor, because I prefer immutable classes. It would be a breaking change now, but we can still add [Obsolete] to the setters. I'm also not sure about the need for all those overloads: couldn't we just have one?

   public BackingFieldAttribute(
       string? name = null,
       BackingFieldAccessibility accessibility = BackingFieldAccessibility.NotApplicable
   )
   

   Are you trying to avoid typing the name of the parameters?

4. I think I tried that but there was some limitation. Not sure if you looked into that, but I can check it again too.

5. Right, I think this is low priority. The compilation error should be fine, no? What would be the advantage of reporting a warning (or even an error) in the generator?

6. I actually came back to this repo because of this issue. I will try to fix it.

7. It would be nice, but not sure if it is high priority. Would have to check if it's actually worth it.

8. That would be nice to have. Both suggestions look good: the first one probably helps those typing nameof in the IDE, while the second one sounds better. I would probably go with the first one, though.

[jp2masa]

Quick update:

• Completed: 4 (partially), 6, 7
• Pending: 3, 8
• To think/discuss further: 1, 2, 5

I added static classes to the candidate types, partially fixing 4, but it may not be the ideal solution. May need to revisit it.

@Athari Would you like to work on some of the remaining items?

[Athari]

1. System.ComponentModel attributes

System.ComponentModel attributes are kind of special (used for serialization and other stuff, I think?), so I prefer not to mess with them.

These attributes aren't exactly special. A huge portion of them is a part of WinForms which Microsoft thought would be "framework-worthy" and not too WinForms-specific. These are technically meant for design-time UI and stuff. The read-only attribute is one of those.

The special attributes are in the System.Runtime.CompilerServices, System.Diagnostics.CodeAnalysis etc. There's System.Runtime.CompilerServices.IsReadOnlyAttribute, for example, but the compiler won't even let you use it in the code. The actual read-only-ness is specified via the initonly in IL metadata, so that's not an option too.

7. Incremental source generator

There's more to incremental source generators than implementing a different interface. There's a huge guide here: Creating an incremental generator.

If I understand it correctly, to achieve maximum performance (as in, avoid unnecessary executions), it's necessary to:

1. Split filtering of targets into multiple steps, with the first step being dumb (typically based on an attribute) and the second being smart.
2. The pipeline steps needs to be cacheable (typically using arrays with sequence equality).

That guide also includes recommendations on how to deal with attributes to avoid the mess with InternalsVisibleTo, which seems rather important.

I added static classes to the candidate types, partially fixing 4, but it may not be the ideal solution. May need to revisit it.

There's a risk this fix in particular could kill performance in huge projects where the number of types is significant, as static types can be abundant.

It may be a good idea to introduce an opt-in attribute for types. Processing all inheritors of AvaloniaObject is probably a good way to filter types, as there's a really high likelihood that these require property generation, but I'm not 100% sure it's as fast as checking an attribute, which is specifically included in the newer API.

3. BackingFieldAttribute constructor

Are you trying to avoid typing the name of the parameters?

Well, yes. With just 2 parameters, it seems unnecessary to specify names.

8. nameof for Attached properties

That would be nice to have. Both suggestions look good: the first one probably helps those typing nameof in the IDE, while the second one sounds better. I would probably go with the first one, though.

NameOfProperty also has the benefit of being unlikely to cause name conflicts as I doubt anyone uses names like these. 😁

It's also possible to make it an option, see Accessing MSBuild properties and user configuration from source generators. So maybe <AvaloniaAttachedPropertyNameTemplate>NameOf{0}</AvaloniaAttachedPropertyNameTemplate> or something, with empty string disabling the generation of these.

Would you like to work on some of the remaining items?

Probably later. I'm busy with some other stuff.