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

Issue/1062 #1069

Merged
merged 6 commits into from
Dec 21, 2023
Merged

Issue/1062 #1069

merged 6 commits into from
Dec 21, 2023

Conversation

GrahamTheCoder
Copy link
Member

@GrahamTheCoder GrahamTheCoder commented Dec 21, 2023

Fixes #1062

@StealthyDeveloper I don't suppose you remember or can see the context of why the code in d34ac82 was added?
It feels like it was trying to avoid a name clash, ambiguous invocation or infinite reference loop, but when I remove that code nothing seems obviously wrong.

I've tested one case here which seemed fine for example

@StealthyDeveloper
Copy link
Contributor

StealthyDeveloper commented Dec 21, 2023

@GrahamTheCoder Hi! Yes that code was fixing this bug: #749. Additionally, if memory serves correctly c# disallows optional ref arguments. This applies to the attribute as well as demonstrated by the two added cases here.
Perhaps emitting a parameterless overload in this case is safest?

Non ref optional parameters should be preserved if they aren't indeed.

@GrahamTheCoder
Copy link
Member Author

GrahamTheCoder commented Dec 21, 2023

Ah yes, thanks. Reading my own message I was a bit vague - you correctly realised it's the removal of the optional attributes I was referring to.
Since the attributes are allowed in C#, we might as well keep them since I believe VB projects will be able to call with the old signature (and the code as it was removed all optional parameters, not just ref ones).

But I see from your example and explanation we'll want converted calls that omit arguments to keep working.
Creating overloads without various parameters would combinatorially explode for several parameters. I think the best thing is probably to inline the default parameter value at each call site.

I was going to create a separate issue for that, but found it was already fixed in #91 and has a test in the repo

This VB seems to convert into something compilable for example.

Let me know if you can see any situation where the output won't compile - apologies if I've misunderstood your point.

@GrahamTheCoder GrahamTheCoder merged commit 3284c3d into master Dec 21, 2023
@GrahamTheCoder GrahamTheCoder deleted the issue/1062 branch December 21, 2023 23:57
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.

VB -> C#: explicit interface implementation looses parameter optionality
2 participants