Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

VB -> C#: Control names are renamed (name ==> _name only in specific forms) #615

Closed
marcal opened this issue Aug 18, 2020 · 18 comments
Closed
Labels
output logic error A bug where the converted output behaves differently to the input code VB -> C# Specific to VB -> C# conversion

Comments

@marcal
Copy link

marcal commented Aug 18, 2020

Hello,

Sometimes the controls names are renamed into private and public properties of the name are created.
The problem is when you use the GUI to create columns in a datagridview (there is an error column doesn't exist).

Input code

// In the InitializeComponent
dim trvUse as TreeView 

Erroneous output

private TreeView _trvUse;

Expected output

private TreeView trvUse;

The name of the control shouldn't change

Details

  • Product in use: e.g. .NET extension
  • Version in use: 8.1.7 (last at the moment
  • Did you see it working in a previous version, which? No
    In some forms the controls are renamed (a "_" in added in front of the controls.)
    It is not a big problem but if I have a datagridview where the columns are added in the GUI (I add a column Txt for example), the column, is also renamed in _Txt.
    After that when I access the cell value (dgvTest.Rows(10).cells ("Txt").Value) there is a crash as the column is now named _Txt

I am sorry because I can't make a repro (I don't understand why in a smaller project I don't have the error.)
I am still trying to do something. I will update in case I manage to make a repro.

@marcal marcal added the VB -> C# Specific to VB -> C# conversion label Aug 18, 2020
@GrahamTheCoder
Copy link
Member

GrahamTheCoder commented Aug 18, 2020

Thanks for raising this. Should be enough info to repro. The renaming of the field is done when there is a method which "handles" one of its events.
The converter should be adding code to the constructor to set the Name property back to the name without an underscore though, so an exception shouldn't have happened.
I did change this behaviour recently so it's very possible there's a case where the name fixups don't happen

@marcal
Copy link
Author

marcal commented Aug 19, 2020

Hello,

OK. In the converted code, there seems to be some names that don't have the _ (4 or 5) so perhaps there is something that stopped the process to remove the _.

Marc

@GrahamTheCoder
Copy link
Member

I have failed in my attempt to reproduce this. See dae47c3#r42097263

Let me know if you have any other information, or if you've seen it since #618 was fixed, thanks!

@marcal
Copy link
Author

marcal commented Sep 8, 2020

Hello,

The problem is still there but it is less important. Now the DataGridViewTextBoxColumn are not renamed anymore (so there is no crash in runtime too). Some controls are still renamed but it doesn't matter.
What is strange is that taking the code of the form designer only makes no problem. Even if it is not a problem for the runtime (no crash anymore in exe), I have added a repro for the name of the controls in case you still want to solve it.

@marcal
Copy link
Author

marcal commented Sep 8, 2020

Hello,
I have managed to make a repro.
Repro.zip.
Unfortunately, it is big. If I remove the code from the form, the problem disappears so I have to let it.
You will see a lof of objects being renamed (cboLng becomes _cboLng)...

@GrahamTheCoder
Copy link
Member

GrahamTheCoder commented Sep 8, 2020

Thanks for that. I think I've managed to get a small repro. I'll leave it here to remind me when I have some time to dig into what's happening.

<DesignerGenerated>
Class AnyClass
    Private WithEvents cboLng As DataGridViewTextBoxColumn

    Public Sub SomeMethod() Handles cboLng.Dispose
    End Sub
End Class

Expected (should have Name property set):

using System.Runtime.CompilerServices;

internal partial class AnyClass
{
    private DataGridViewTextBoxColumn _cboLng;

    AnyClass()
    {
        _cboLng.Name = "cboLng";
    }

    private DataGridViewTextBoxColumn cboLng
    {
        [MethodImpl(MethodImplOptions.Synchronized)]
        get
        {
            return _cboLng;
        }

        [MethodImpl(MethodImplOptions.Synchronized)]
        set
        {
            if (_cboLng != null)
            {
                _cboLng.Dispose -= SomeMethod;
            }

            _cboLng = value;
            if (_cboLng != null)
            {
                _cboLng.Dispose += SomeMethod;
            }
        }
    }

    public void SomeMethod()
    {
    }
}

Actual (constructor not even created):

using System.Runtime.CompilerServices;

internal partial class AnyClass
{
    private DataGridViewTextBoxColumn _cboLng;

    private DataGridViewTextBoxColumn cboLng
    {
        [MethodImpl(MethodImplOptions.Synchronized)]
        get
        {
            return _cboLng;
        }

        [MethodImpl(MethodImplOptions.Synchronized)]
        set
        {
            if (_cboLng != null)
            {
                _cboLng.Dispose -= SomeMethod;
            }

            _cboLng = value;
            if (_cboLng != null)
            {
                _cboLng.Dispose += SomeMethod;
            }
        }
    }

    public void SomeMethod()
    {
    }
}

@GrahamTheCoder GrahamTheCoder added the output logic error A bug where the converted output behaves differently to the input code label Oct 9, 2020
@GrahamTheCoder
Copy link
Member

GrahamTheCoder commented Oct 12, 2020

Ah, turns out my repro was wrong. Here's my new understanding:

  • The "Name" of a Component is string.Empty until it's set somewhere.
  • When the converter converts InitializeComponent, it sets the name to the one with an underscore: This is necessary for the visual designer to correctly associate with handling methods.
  • Therefore, only Components with their name set (and changed by the converter) in InitializeComponent should have their name overwritten in the constructor (in a non-generated file wherever possible)

@docfresh
Copy link

This happened on my project in VB -> C# on CC 8.40 and I'm not sure how to clean up the mess. Renaming the components in the designer brings up a slew of "ambiguous reference error" and now I'm stuck with different naming conventions for controls prior to conversion, and after. There's a mix of those with and without the _ prefix.

image

@docfresh
Copy link

Here is a solution that seems to work.

  1. Go into designer.cs and delete the 'internal' stuff under the private declaration

private TextBox _txtWeeklyReportsEmails;

    internal TextBox txtWeeklyReportsEmails
    {
        [MethodImpl(MethodImplOptions.Synchronized)]
        get
        {
            return _txtWeeklyReportsEmails;
        }

        [MethodImpl(MethodImplOptions.Synchronized)]
        set
        {
            if (_txtWeeklyReportsEmails != null)
            {
                _txtWeeklyReportsEmails.TextChanged -= settingsChangedEvent;
            }

            _txtWeeklyReportsEmails = value;
            if (_txtWeeklyReportsEmails != null)
            {
                _txtWeeklyReportsEmails.TextChanged += settingsChangedEvent;
            }
        }
    }

THEN you can rename the component on the designer to remove the leading underscore _.

If you rename in the designer first, you get messy errors.

@GrahamTheCoder
Copy link
Member

The converter needs to add those properties since that's what VB does behind the scenes. It tries to only add them if they are needed to assign events - in order to reduce the amount of noise on the output. If you delete the internal property (and rename the field) you'll find that anywhere that assigns the field no longer reattaches the events.

If it wasn't being assigned to (except in initialize component) , let me know - I can't remember if the converter already checks that too. The converter could in future avoid generating the property in such cases.

You're right the names are inconsistent in one sense, but the consistent thing is that you should always use the non-underscore version in your code (whether it's a field or property).
This is a useful kind of consistency and the best that's possible given the constraints.

Obviously if you dislike underscores as a personal preference, then you can rename the ones prefixed with an underscore to literally anything that's valid... I. E. Not the name of something else that already exists.

@docfresh
Copy link

docfresh commented Dec 14, 2021

Thank you for responding. Are you sure there is a problem with the attachment of events? In a control where I deleted the 'internal' structure, and renamed the field without the underscore, the settingsChangedEvent still fired as intended. Perhaps I am not understanding you correctly as to what the problem is. I would like to understand further, because I am about to spend a lot of time renaming hundreds of controls like this.

My biggest complaint is that this behavior prevents me from renaming controls. It makes me feel out-of-control with my own code. Additionally, it also prevents you from navigating the Properties dropdown box by typing the first name of the control. It also manifests when you copy/paste controls from one form to another (the underscore comes with the controls).

@GrahamTheCoder
Copy link
Member

GrahamTheCoder commented Dec 17, 2021

If the WithEvents field is only assigned in InitializeComponent, or you were manually adding any event handlers there may be no issue. The easiest way to understand what's happening is to compile your VB solution, delete the pdbs, and open in DotPeek. Then you'll see a csharp interpretation of what the VB compiler does.

I started spiking a fix for this yesterday. It's quite a tricky set of constraints but I'm reasonably confident in the solution. I'll try to complete it next week but can't promise anything.

@GrahamTheCoder
Copy link
Member

Disregard my above comment about spiking. I got mixed up, it was meant in regard to issue #774 (which is the bug that would happen everywhere if not for the existing property/field pair generation).

@GrahamTheCoder
Copy link
Member

I have made some changes as part of #774 which should decrease the number of cases in which future conversions create the delegating property
In particular, if there is no write-usage of the property (except InitializeComponent) and no inheriting class, then it should no longer create the property - it should use the field directly. I hope that helps with your particular case!

@docfresh
Copy link

Thanks @GrahamTheCoder . I am sure it will be helpful. However I have already made many modifications to the converted project and can't go back to the original VB source. It looks like I will need to find a way to rename all the form controls by hand. Yet, I think others will appreciate it!

@GrahamTheCoder
Copy link
Member

If you have a git commit from immediately after the initial conversion (before manual changes), it's possible you can start a new branch there, redo the conversion on top of that with a new version, then merge it with the branch containing manual changes. Depends a lot on what git history you have and how the changes overlap - just mentioning since it worked for me in a similar situation

@grochoge
Copy link

@GrahamTheCoder I'm running into issues with _ being prepended to the fields in a WinForms control. This is changing the automation ID of everything which causes the references in our automated UI tests to no longer be valid.

I tried installing CI build #290 but it's still renaming fields and generating properties even though nothing is setting the property (only InitializeComponents setting the underlying field). It does have events; does that force property generation?

@GrahamTheCoder
Copy link
Member

GrahamTheCoder commented Feb 12, 2022

Thanks for getting in touch - sorry to hear the underscores are appearing unexpectedly. All changes discussed previously are in the latest build, so there should be no need for any CI builds. Using the latest released version from the marketplace. The short version of what I want to ask you, is do the types with transformed property have any subclasses within the solution? If yes, you can probably skip the rest of the details below.

Otherwise, could you pick one field that's definitely incorrect and carefully check through the conditions for this logic I'll describe (the main details of) below? i.e. Let me know if the VB meets the conditions and whether you're seeing the output described.

https://github.com/icsharpcode/CodeConverter/blob/master/CodeConverter/CSharp/DeclarationNodeVisitor.cs#L216

Within type T, member P gets transformed if all conditions are true:

  1. P is a property that is used in a Handles statement of a method within the type
  2. T has no type that inherits from it
  3. M is written anywhere other than InitializeComponent:
    • It finds InitializeComponent by checking if type symbol for T has the designer generated attribute. If so it finds the first 0 parameter non-shared Sub called "InitializeComponent" accessible within the type (within T or any of its base classes - including any ancestors declared in VB in this solution)

If transformed:
a. A backing field _P is created
b. The setter for P references _P but unassigns/assigns the handled events
c. InitializeComponent uses _P directly to keep the C# visual designer working (it can't use properties)
d. In the non-designer-generated part of the type, the constructor should gain some assignment statements which set the P.Name ="P"

I believe the conditions above are generally an over-estimate of when the transformation is required. In particular, having a subclass for a type isn't sufficient to say a transformation is needed (it's more about whether the property is actually overridden within those types). That area can obviously be improved if that's the issue you're facing, but it could also be that there are issues with the implementation of the concepts outlined above.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
output logic error A bug where the converted output behaves differently to the input code VB -> C# Specific to VB -> C# conversion
Projects
None yet
Development

No branches or pull requests

4 participants