RegisterProperty Updated for nameof()

[JasonBock]

I noticed this morning that there's no overload to RegisterProperty that takes a string (at least I didn't see one). I understand why expressions were used, but now that C#6 has nameof(), having the ability to specify the related property with a string may give a slight perf improvement as runtime expression parsing isn't needed. An analyzer can be used to enforce that a developer should use nameof() to get the name of the property, and can also start discouraging the use of expressions (both can be warnings).

[rockfordlhotka]

I think that's why there's no (public) string overload - without analyzers there wasn't a way to tell people not to use such a thing.

I like your proposal - though that second warning would be a pretty major thing for all existing codebases. I wonder what other people think about that?

[JasonBock]

The 2nd warning could be labeled as an Info. There's so much code out there that have used expressions it would be pretty harsh to label it as a Warning, at least with the first release. After a while, maybe bump it up to Warning. But have....something to inform the dev that using the expression approach is no longer preferred with C#6 and nameof().

[ajj7060]

Please don't add a warning or info for using expressions in RegisterProperty/RegisterMethod yet. nameof is way too new and we just got our warnings down to less than 10; warnings or info for use of expressions will just add a ton of noise. Wouldn't the overloads using expressions need to remain for people that aren't on VS2015 yet anyway?

[rockfordlhotka]

Additionally, this new pattern can only be widely used once all supported platforms can use it (Android, iOS, UWP, .NET, mono, etc.).

[JasonBock]

ajj3085 all this would be for 4.6 code only. You don't get the analyzers with any previous versions. Also, you can easily disable analyzers if you want to.

[rockfordlhotka]

Do we know if C# 6 features are in Xamarin at this point?

[JasonBock]

I don't know. @Bowman74 may know :). Here's one recent post:

https://blog.xamarin.com/xamarin.studio-5.9-enhancements/

[JasonBock]

It would also be good to see how much of a perf benefit you get using a string compared to parsing the expression to get the string. My guess is you should get one, but it would be interesting to discover how much.

[rockfordlhotka]

I suspect it will be minor since these are static fields that are initialized exactly once per AppDomain lifetime.

[ajj7060]

@JasonBock Wouldn't the analyzers run if you're using VS2015 but still targeting Net 4.0? I believe the newer compilers are still used even when targeting older versions of the framework. Or does Roselyn only work for .Net 4.6+?

[JasonBock]

That's a great question :). I really don't know. I thought the new compiler stuff is only for .NET 4.6. They definitely have support for new code features like nameof(), which wouldn't work in previous versions. So I'm guessing VS2015 doesn't use the new compilers for projects targeting previous versions, but again, that's just speculation.

[ajj7060]

@JasonBock It looks like it is the case that you can use the new C# 6 features even when targeting older version of the dotnet framework: http://stackoverflow.com/a/28921749/347348 Still, I my preference would be to not include analyzer warnings (yet) for using the expression overloads on RegisterProperty; maybe at some point, or if you can selectively enable / disable analyzer rules, but if they always show, I think its a bit premature for this case.

[JasonBock]

But that's the thing, you can easily disable analyzers. And I personally would want to know if there's a better way to do something. If it's an Info, I'll probably never see it. A Warning, I would, because I turn warnings into errors ;).

@rockfordlhotka could also consider obsoleting the expression approach, but that will probably never happen for legacy reasons.

[ajj7060]

@JasonBock If its possible to selectively disable the analyzer checking for nameof, I wouldn't object to that, but our code base still has criteria classes that are pocos with public fields (that don't work with the httpproxy channel), and it was only 7 months ago we went from Csla 3.8 to 4.5, just to give an example of how slow getting things up to the most current spec can be. And the other analyzers I've seen tickets for sound useful, so if its all or nothing for analyzers the nameof one should wait, IMO.

[JasonBock]

On other thing I thought of....a refactoring could also be added for code that currently uses RegisterProperty with expressions. So instead of it being a diagnostic (at least to start) it would show up as a refactoring. This isn't as obvious because a dev has to do Ctrl+. to see the refactoring options. If they don't do that, they'll never get that. But it's something to think of.

[rockfordlhotka]

Would that show the blue squiggley?

[ajj7060]

I finally found where you can turn these on or off. Very cool. It would be great if a refactoring could be applied across the entire project / solution automatically.

[JasonBock]

Well, it's 2019....and I think at this point, nameof() should be pretty common :). I think this should be revisited. A couple of things to discuss:

• See just how much time is saved by using a string over parsing an expression (Benchmark.NET comes to mind here)
  • Introduce an overload that takes a string
  • Provide an analyzer that would detect if a literal string is passed in, and suggest using nameof() instead. This would just be an Info.
  • Provide an analyzer that would look for calls to RegisterProperty that pass in an expression, and flag it as an Info or Warning (TBD). Code fix would be to change it to a nameof() call (not sure I can do that, but it should be possible).

[rockfordlhotka]

@JasonBock can you show some code as to what people would be typing after this change?

[JasonBock]

Something like this:

public static readonly PropertyInfo<Guid> IdProperty =
	RegisterProperty(nameof(Id));
public Guid Id
{
	get { return this.GetProperty(IdProperty); }
	set { this.SetProperty(IdProperty, value); }
}

This doesn't work right now. The closest I think you can get is this:

public static readonly PropertyInfo<string> FirstNameProperty =
	RegisterProperty(new PropertyInfo<string>(nameof(FirstName)));
public string FirstName
{
	get { return this.GetProperty(FirstNameProperty); }
	set { this.SetProperty(FirstNameProperty, value); }
}

I wonder if this will get rid of a weird inheritance issue that CSLA has with registering properties: http://dotnetbyexample.blogspot.com/2010/08/fixing-clsa-property-registration.html. I haven't looked at this in a long time though.

[rockfordlhotka]

Locking this thread so all conversation occurs in the actual work item.