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

Update PXOverride diagnostic's messaging to indicate that a signature was not found #504

Open
ShadowFiendZX opened this issue Jun 23, 2023 · 3 comments
Labels
enhancement New feature or request

Comments

@ShadowFiendZX
Copy link
Collaborator

ShadowFiendZX commented Jun 23, 2023

I made just made a mistake in my PXGraphExtension hierarchy that led to me getting the new diagnostic error about a PXOverride signature mismatch. (Although I don't remember updating Acuminator recently, as I install it by building it locally ever since I first starting contributing, and I'm rather embarrassingly out of date)

To put it simply, my implementation was something like this :

// Acumatica's graph
public class APPaymentEntry : PXGraph<APPaymentEntry>
{
}

// Arbitrary extension 1 that adds a virtual method.
public class APPaymentEntryExtension1 : PXGraphExtension<APPaymentEntry>
{
    public virtual void DoSomething()
    {
    }
}

// 2nd extension that I accidentally made extend itself due to lazily relying on VS autocomplete
public class APPaymentEntryExtension2 : PXGraphExtension<APPaymentEntryExtension2, APPaymentEntry>
{
    public delegate void DoSomethingDel();

    [PXOverride]
    public virtual void DoSomething(DoSomethingDel del)
    {
        del();

        // Arbitrary work
    }
}

In this case I got the error The signature of a method with the PXOverride attribute must match the overridden method
While it is a problem that I made my 2nd extension extend itself, which would've likely led to errors when loading Acumatica, and might be an idea for a new diagnostic in itself, it took me a few minutes of staring at all my code to realize what I did wrong.

I think it'd be a little more helpful if the analyzer in this case would say something like A method with this name was not found in the base graph or referenced graph extensions "APPaymentEntryExtension2". This would be a separate case from a signature mismatch.
The original error would still show if a method with that name was found, but like the error states, it'd show when there is a signature mismatch, just not when the signature is missing entirely.

However, I'm not sure how easy this would be, as I'm not sure if Acumatica supports PXOverriding methods from other extensions that aren't directly referenced in the extension list PXGraphExtension<...>.
I'm also not sure how this would pan out for generic classes, as I haven't looked into whether or not analyzers can (or do) resolve generic parameters.

For example :

public abstract class SomeGraphExtension<TGraph> : PXGraphExtension<TGraph>
where TGraph : PXGraph, new()
{
    public virtual void SomeMethod()
    {
    }
}

public abstract class SomeGenericExtension1<TExtension1, TGraph> : PXGraphExtension<TExtension1, TGraph>
where TGraph : PXGraph, new()
where TExtension1 : SomeGraphExtension<TGraph>
{
}

If this is not possible, feel free to close this issue, I was just hoping there'd be some way to help others more quickly resolve the mistake I made.

@ShadowFiendZX ShadowFiendZX added the enhancement New feature or request label Jun 23, 2023
@SENya1990
Copy link
Contributor

Hi @ShadowFiendZX .
Visual Studio usually updates Acuminator itself since we increment Acuminator version each release. When VS sees a new version it updates Acuminator.

Regarding your suggestion:

I think it'd be a little more helpful if the analyzer in this case would say something like A method with this name was not found in the base graph or referenced graph extensions "APPaymentEntryExtension2". This would be a separate case from a signature mismatch.
The original error would still show if a method with that name was found, but like the error states, it'd show when there is a signature mismatch, just not when the signature is missing entirely.

We cannot list all extensions in the error message because the list of base extensions can be arbitrary long. Moreover, the for each level of chained graph extensions (except the last one) we should take into account base graph extensions. For example:

public class MyGraph : PXGraph<MyGraph> { }

public class GraphExt1stLvl : PXGraphExtension<MyGraph> {}

public class DerivedGraphExt1stLvl : GraphExt1stLvl {}

public class GraphExt2ndLvl : PXGraphExtension>DerivedGraphExt1stLvl, MyGraph> {}

public class DerivedGraphExt2ndLvl : GraphExt2ndLvl  {}

// and so on

We can distinguish the case when there is no method to override form the signature mismatch. Perhaps, it will make the diagnostic more convenient, But we won;t list names of all base extensions.
Let's keep this as a change request.

@SENya1990
Copy link
Contributor

@ShadowFiendZX I have a feeling that what you really need is not the enhancement of this diagnostic but another check that will validate that graph extension does not pass itself to the list of base extensions. If yes, then please create another issue for such diagnostic.

@ShadowFiendZX
Copy link
Collaborator Author

@SENya1990 I've created this issue requesting a new diagnostic for the circular reference mistake.

I'm not really sure that the change I've requested here would be as useful if the other new diagnostic is created, since in that case with this diagnostic's current message you'll probably look at the base method either way, and would eventually realize you misnamed your PXOverride.
But if you think the change requested here (ignoring the part that asks for listing the extensions) would prove beneficial to developers, you can keep this issue open, otherwise you can close it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants