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

Source Generator #18

Draft
wants to merge 27 commits into
base: dev
Choose a base branch
from
Draft

Conversation

Guiorgy
Copy link
Contributor

@Guiorgy Guiorgy commented Feb 25, 2024

Goals:

  • Close Source Generators #17
  • Add an attribute that accepts a set of values to find and replace with, as well as a set of regex patterns and their replacements.
  • Generate copies of the decorated type declarations and performe the defined replacements.

Imlementation:

  • a new build target DebugGenerators is added that extends the Debug target. This is so we can selectively call if (!Debugger.IsAttached) Debugger.Launch(); to debug the generator execution only when the DEBUGGENERATORS symbol is defined. This is not the only way to debug generators, but it's the best one I know.

  • a public internal attribute by the name GenerateCopyAttribute is generated in the SpanExtensions.Enumerators namespace. It can be used in the following way:

    [GenerateCopy(FindAndReplace = new[] { "oldValue1", "newValue1", "oldValue2", "newValue2" }, RegexReplaces = new[] { 
    "pattern", "replacement" })]
    public class Something {...

    The attribute accepts arrays of strings, one for normal string find and replaces, and another for regex patterns and replacements. It is not necessary to define both, but at least one of them has to be non-empty, otherwise a build error will be shown:
    image

    Ideally, the parameters should accept array of tuples of find&replace/pattern&replace, however .NET currently doesn't support that, thus the attribute accepts a normal array of strings, however, it assumes that every odd string is the find/pattern and every even string is a replacement, thus if the length of the array is not even, a build error is displayed:
    image

  • If the marked class is a partial, the output generated file includes the original source file at the end of its name. This is to avoid collisions when a partial class is spread into multiple files and marked by the attribute in every file. This logic can still fail, for example, if the class is separated into files with the same name but in different subdirectories. Bcause of that, an additional parameter is avaliable, GeneratedFileTag, which will be used at the end of the output file name (instead of the original source file name), which can be used when a collision happens.

  • The generator currently assumes that the same class in the same source file won't be marked by the attribute more than once. While this is an unlikely situation, it is nevertheless recommended to mention this in the documentation for future contributors.

  • The generator currently assumes that namespaces are not to be modified in the copy, thus the namespace and using statements are copied untouched. This may not be the case in the future, if it's decided to spread some implementations across namespaces, but for now it is ignored to simplify implementations.

  • Another thing that might be worth mentioning to future contributors is to pay attention to the replacement statements at the top of the class. For example, if the regex (?<!ReadOnly)Span -> ReadOnlySpan is used and a developer later adds a call to .AsSpan, that will be replaced with AsReadOnlySpan. In almost every case this can be solved by using a different approach (the developer might add an extension method AsReadOnlySpan in our example), or modifying the regex pattern ((?<!ReadOnly|As)Span in our example), but is still something that should be kept in ming during development.

@Guiorgy
Copy link
Contributor Author

Guiorgy commented Feb 25, 2024

Before I continue, can you comment on the naming and project structure? If everything looks file, I'll continue.

PS. It shouldn't take long to get a working generator, there's just a few things missing.
Edit: It should be basically done. Just need to fix documentation comments, for some reason new lines are added between them...

@dragon7307
Copy link
Member

Ok, I'll look at it, as soon as I am done with my review of the Tests..

As a side note: Can't the tuples be fixed by just accepting a struct that has two fields and can implicitly be converted to and from a tuple????

@Guiorgy
Copy link
Contributor Author

Guiorgy commented Feb 26, 2024

Here's a quote from docs:

The types of positional and named parameters for an attribute class are limited to the attribute parameter types, which are:

  • One of the following types: bool, byte, char, double, float, int, long, sbyte, short, string, uint, ulong, ushort.
  • The type object.
  • The type System.Type.
  • An enum type, provided it has public accessibility and the types in which it is nested (if any) also have public accessibility.
  • Single-dimensional arrays of the above types.

So, while an attribute can accept anything as a base object, this would handwave any type safety and correctness, and even if you accept that, don't forget that we are using the attribute in a source generator, not in reflection, in other words, what we get is the source code inside the attribute brackets, not the actual objects passed, so parsing that would be... painful...

Edit: Also, when I said basically done I meant the generator itself, I still need to migrate everything to be using the generator, though, I also want to test it in different conditions just in case, so if you want you can play around with it and tell me if you find any problems ;)

Edit2: Span<T> doesn't seem to have .Trim() in .NET 5, and no .IsWhiteSpace() at all? Well, we can add internal extensions to cast Span into ReadOnlySpan and call the builtin extensions I guess. Added

Edit3: A problem I run into while testing is that if we decorate a partial class that is spread into several files, having an attribute with AllowMultiple = false means that only one of those files can be copied. And if we do AllowMultiple = true, the generator assumes that the same class (in the same file) won't be decorated by that attribute several types, not sure what exactly will happen, but pretty sure something will fail. Either way, I doubt someone will decorate the same class with the same attribute several times, but I'd recommend mentioning that in the wiki for future contributors.

Edit4: on my source-generators-pr11-tests testing branch I managed to build and run tests for both Span and ReadOnlySpan with only sources for Span in the projects.
The regex patterns used:

  • "(?<!ReadOnly)Span" -> "ReadOnlySpan" (Any "Span" that isn't preceeded by "ReadOnly", used in main project)
  • "(?<!ReadOnly|As)Span" -> "ReadOnlySpan" (Any "Span" that isn't preceeded by "ReadOnly" or "As", used in tests)
    Normal find and replaces used:
  • ".ToCharArray().AsSpan()" -> ".AsSpan()" (used in tests)

Edit5: I'm also considering adding a small generator to generate ToSystemEnumerableExtensions for every split enumerable, since that's just a bunch of boilercode, but honestly, might be simpler to just keep things as are?

Edit6: I think it would be easier to wait for the other 2 PRs to be properly merged before merging this, that way I could modify all 3 projects to use this generator. If you'd like, you could add me as a collaborator temporarily, and I could deal with the merge conflicts and merge the 3 PRs?

@Guiorgy Guiorgy marked this pull request as ready for review February 26, 2024 20:32
@dragon7307 dragon7307 added this to the Backlog milestone Feb 28, 2024
@Guiorgy
Copy link
Contributor Author

Guiorgy commented Sep 1, 2024

Since opening this PR, I've run into this amazing article on C# Source Generators by Andrew Lock, the creator of EnumGenerators. There he describes different methods to improve Source Generator performance, usability and testability, for example, a way to output generated files to be stored in version control (git) to compare/validate changes.

I'm thinking about rewriting this PR when I have time. If you like the idea, I'll make it into a draft and work on it later?

@dragon7307
Copy link
Member

Yeah, that'd be great. I just came around (after a really long time) to finishing the unit-tests and implementing the updated Split methods as well as some new additions. Planning to release v1.4 with those features next week.

PS:
The article truly seems amazing. Gotta have a more in-depth look at it soon!!

@Guiorgy Guiorgy marked this pull request as draft September 2, 2024 09:29
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.

2 participants