@AnthonyDGreen

Think the issue is in the MethodSignatureComparer here

Since it now possible for the parameter to be IsOptional = True and not have an HasExplicitDefaultValue = False. So we need a way to fix it.

Looking at HasExplicitDefaultValue it's not clear if the purpose of this flag is about the syntax or more what's in metadata. So, even with your proposed feature the emitted metadata for a default value should look indistinguishable from what would be emitted were the user to have written "= Nothing". So if you round-trip a method symbol from source to metadata this flag will have HasExplicitDefaultValue = True on the import side. I'm inclined to not change this flag but couldn't track down the consequences if we leave it True without corresponding syntax.

@AnthonyDGreen
My understanding it that HasExplicitDefaultValue is related to source. I think these 2 failures, as the other emit test are successful, are to do the signature comparison. Which is currently expects that the parameters have a explicitly stated value Eg = Nothing or = "" or = 0.
I'm currently trying a version with multiple if eg the other three cases.

I'll do a push in bit.

Somehow my comment about Binder.GetArgumentForParameterDefaultValue disappeared. So I will expand on it and summarize my overall thinking about the current feature implementation.

I think that the feature should be just a syntactic relaxation. Meaning that, if optional parameter declaration is omitting the default value, then the behavior should be as if the value was explicitly specified to be NOTHING (NOTHING literal). We should also make sure that:

• There are no differences in consumer behavior regardless whether the declaration comes through compilation reference, or through metadata reference.
• Other consumers of such parameter (VB15 compiler, C# compiler, F# compiler and other compilers out there) would treat it exactly like if the value was explicitly specified to be NOTHING (NOTHING literal).
• VB compiler with feature enabled wouldn’t interpret the same metadata differently by comparison to VB15 (a breaking change otherwise).

Given this, I think we cannot rely on the feature availability check (at least the way it is done right now) in Binder.GetArgumentForParameterDefaultValue for correct behavior.
Here are some reasons why:

• The parameter could be declared in a different compilation, whether the feature is available at the consuming compilation shouldn’t make any difference on what value is used at the call-site.
• I haven’t seen any changes to parameter symbol behavior. This means that in metadata parameter won’t have any explicit default value. Whether there is an explicit parameter default value in metadata makes a difference in what value is used at the call-site. Since VB15 compiler wouldn’t have the change in Binder.GetArgumentForParameterDefaultValue, it, therefore, could use different default values by comparison to VBNext compiler.
• As mentioned before, whether there is an explicit parameter default value in metadata makes a difference in what value is used at the call-site. The special case around feature availability will likely cause different default values used in VB15 vs. VBNext for the same metadata existing today, which will likely be a breaking change in behavior.

It seems, the simplest way to implement the feature with required behavior would be to adjust behavior of parameter symbol so that, instead of “saying” that it doesn’t have an explicit default value, it would “say” that it does have an explicit default value (despite the fact that the value isn’t specified in the syntax). That default value must exactly match default value corresponding to NOTHING literal.

@AlekseyTs
It will "disappear" because it was a comment associated with an old version of PR code,

I think that the feature should be just a syntactic relaxation. Meaning that, if optional parameter declaration is omitting the default value, then the behavior should be as if the value was explicitly specified to be NOTHING (NOTHING literal). We should also make sure that:

It is a syntactic relaxation, but additional changes are required because the prior existing code currently assumes the "explicit parameter initializer" is not null.
This PR changes it, to be aware that it could be null.
In the case where isn't an explicitly stated "explicitly stated default value" it is treated as if there is an
= Nothing,

With the feature present is does still passes the prior existing metadata unit test.

I haven’t seen any changes to parameter symbol behavior. This means that in metadata parameter won’t have any explicit default value. Whether there is an explicit parameter default value in metadata makes a difference in what value is used at the call-site. Since VB15 compiler wouldn’t have the change in Binder.GetArgumentForParameterDefaultValue, it, therefore, could use different default values by comparison to VBNext compiler.

The prior existing code, assumes the parameter is never null. Hence requires to be change to account that it could be null.

Given this, I think we cannot rely on the feature availability check (at least the way it is done right now) in Binder.GetArgumentForParameterDefaultValue for correct behavior.
Here are some reasons why:

With all due respect, could you provide evidence (eg unit test) for the "reasons why", will have the alleged affects. Then we can be confident that it doesn't break, and maintains expected results.
As I'm confident as implement it does this. Though I'm humble enough to accept irrefutable evidence to the contrary.
Or provide a "fix" for that aspect of the code.

It seems, the simplest way to implement the feature with required behavior would be to adjust behavior of parameter symbol so that, instead of “saying” that it doesn’t have an explicit default value, it would “say” that it does have an explicit default value (despite the fact that the value isn’t specified in the syntax). That default value must exactly match default value corresponding to NOTHING literal.

The branch is Open-Sourced and I'm willing to accept a Pull Request from the community,

With all due respect, could you provide evidence (eg unit test) for the "reasons why", will have the alleged affects. Then we can be confident that it doesn't break, and maintains expected results.
As I'm confident as implement it does this. Though I'm humble enough to accept irrefutable evidence to the contrary.

Let's take a look at the first problem:
"The parameter could be declared in a different compilation, whether the feature is available at the consuming compilation shouldn’t make any difference on what value is used at the call-site."
Since you are saying that the feature availability check is required, it should be very easy for you to find a unit-test that fails if that check is removed. Clone that unit-test and modify it so that the method is now called in the same manner, but from a different compilation, which doesn't enable the feature.

• Compilation Initial port and addition of README.md #1 enables the feature and declares the method,
• Compilation Update links in README.md with ported wiki content #2 doesn't enable the feature, references compilation Initial port and addition of README.md #1 and calls the method without explicit value for the parameter.

Is the same default value used by Compilation2?
Repeat the same with metadata reference rather than with compilation reference.

@AlekseyTs
I'm currently testing a version with the FeatureCheck remove in the Binder_Invocation.
If it works I'll see if it'll also works without the require for Non-Null parseoptions.

If I now revert the requirement for non-null parseoptions. I get these test failures

@AlekseyTs I've managed to remove the requirement for non-null ParseOptions and the FeatureCheck in Binder_Invocation

@AlekseyTs I've updated the unit tests with your recommendations.

Hey Adam,

Thanks again for working on this feature. Thank you to reacting to the feedback and recommendations given thus far. I want the feature and really appreciate you helping out. We’re new to this process of accepting community PRs for language features so I apologize if we haven’t set expectations correctly. I want to try to set some now with the hope that that will make this process smoother moving forward.

First, with regards to compiler changes, as frustrating as it may seem you're being held to the same high standard as any compiler developer working on site here in Redmond. Maybe the bar is a little higher while you’re building a proven track record but not by much. This is the same frustrating experience that all new team members experience. It sucks, I know, I’ve been there. My first few (failed) attempts at contributing to the compilers were epic disasters. The Visual Basic compiler is used to compile millions of programs and we (Microsoft) are responsible for any change that breaks any of those programs. Additionally, any “accidental feature” – where a code change allows something to compile unintentionally is also a problem since it might allow something that doesn’t make sense or doesn’t fit with the language but fixing it in a future release will break new programs written since the “accidental feature” was release. It’s an awesome responsibility and consequently every change to the compiler is met with the highest scrutiny. We want to have the highest confidence that your feature is doing what it’s supposed to do, exactly what it’s supposed to do, and absolutely nothing more that it’s supposed to do. Therefore, in the interest of time every compiler change must be surgical. Whether it’s fixing a bug or implementing a new feature the changes you make must be chosen with surgical precision—touching the exact right lines of code and the smallest number of lines of code overall. When I do either I actually spend a lot more time thinking about where to make the change than making the change itself. Measure ten times, cut once

When you submit a PR the vast majority of our review will be focused on finding red flags in your code. Feverishly finding places where you forgot something or did too much so you can fix it. Sometimes just asking questions to understand why you did something in a certain way or why you didn’t do it another way and whether you’ve thought about a certain test case. Because not understanding why you did something a certain way or that there were other ways to do it and why you didn’t do it that way or not having thought through a lot of test cases to break the feature is a huge red flag. Other red flags are how much change there is in the code-the more the more likely the code is incorrect. That’s why it’s critical that your changes touch the minimal number of files, minimum number of methods, and minimum number of lines of code in those methods. Put another way, the probability of your PR being accepted is inversely proportional to the square of the size of the PR.

Right now here are the red flags I see in your PR:

1. There are 2,654 additions and 663 removals or 3,317 lines of code changed. I haven’t implemented your feature but I have experience in the codebase and my instinct is that you should need to touch < 100 lines of production code to implement this feature with maybe another 100-300 more lines of test code. Because this is a tiny feature. To put it in perspective, I'm not sure the entire string interpolation feature in VB 14 (which I wrote) touches this many files or lines of code. The amount of code you’ve changed is too much. And even if the majority of it is stylistic and not semantic effecting we need to review your code to understand that and it takes exponentially longer to review over 3,000 lines of code changed than 300 lines of code changed. You’re changing 27 files, most of which I know have nothing to do with parsing optional parameters. If you think you can improve the way we check feature availability please send a separate PR for that change with just those changes.
2. You mentioned emit test failing. This feature is a declaration side fix only. No emit code should be broken by it. The expectation is that you loosen a restriction in the parser and make the symbol builder handle the more flexible syntax and maybe harden a few places that for whatever reason reach into the EqualsValue clause of the parameter declaration. The rest of the pipeline should be unaware of any change so if emit is failing something has likely gone horribly wrong in your implementation.
3. You mentioned overload resolution. Overload resolution is the [Spoiler alert] face melting ark of the covenant, see here: https://www.youtube.com/watch?v=n2ZpsbGr7s8 . Any interaction with overload resolution is met with 100x the normal scrutiny. Alarms go off in my office. Jets are scrambled. This is the place in the compiler where the tiniest change can cause the most catastrophic damage. We should all be so fortunate to never have to touch it. And given that your feature is a declaration side feature only and should have 0 impact on the consuming side (which should have no dependency on the syntax used to create the default value) this is actually somewhere between 10 and 20 red flags.
4. There is the appearance of “whack-a-mole” bug fixing. And, I’m not saying you’re doing this only that there is the appearance of it. By that, I mean that some code isn’t doing what it used to, you make a code change to sort of spackle over that bug without clear understanding of the underlying reason for the failure. The fix looks reactionary, not intentional. That’s not a personal attack. I’ve made the exact same mistake before, several times. I implemented the TypeOf … Is … feature several years ago. TypeOf is implemented in terms of TryCast in IL. I found a bug where it was giving an error about not supporting value types or something (because TryCast doesn’t support value types). I understand the language, so I went in, moved the code to implement it in terms of DirectCast (which does support value types) and moved on. Aleksey, says “why’d you make that change?”, I said “to get rid of the error”. Wrong answer. He and I then went and walked through the native C++ code to understand why Roslyn was giving an error in the first place that the native compiler wasn’t. It turns out that the native compiler doesn’t report an error when you classify the conversion (asking if TryCast would succeed). Instead it reports an error when you apply the conversion. A completely different place in the compiler. For the sake of back compatibility, moving the error to the code that applies conversions, instead of the code the checks conversions, was the right fix. Even if my original code got rid of the error and addressed the symptom. The right thing for me to do was to go look at the native code and understand not to lean on my own understanding of the language to unblock myself quickly. We need to have high confidence not only that you’re reducing test failures but that the failures are expected given the changes you should make and that the fixes are the correct changes to restore base line.

Second, you seem to take criticism of your code personally (I’ve also done this). Don’t. Your code is being reviewed by very very senior developers with decades of experience (both individually and collectively) writing compilers and working with these languages. If a Roslyn team member asks a question don’t assume it’s an insult. If they give you advice you should probably take it. You should never invite that team member to send you a PR if they feel strongly about something. You should never demand they provide you with hard evidence that something is a problem. It’s their job to raise questions and point these things out and they’re the gate keepers who have to sign-off of the code and take responsibility for what we ship to customers and support that code later. Always be respectful and act on their feedback. That’s the bar for contributing.

Third, I know we didn’t set this expectation early and that’s our fault but you should actively reach out and solicit opinions on implementation strategies before making them. I can’t tell you how often when writing a feature, I go to @AlekseyTs, or @VSadov, or @cston, or @gafter and ask “I’m thinking of modifying this code here in this way – does that seem like a reasonable approach to implementing this? What problems do you see with that?”. Or I’ll go to @tmat and say “This code works in Edit & Continue, I have no idea why – please explain”. That’s what it means to be a contributor and part of our extended team. And it will save you and epic amount of time compared to doing the work and then defending the code as written. Again, it’s our fault that we didn’t invite you to solicit feedback on implementation strategies before you did all this work. We’re new to this and we tend to make assumptions or forget that the habits we’ve all developed from working with each other for 2, 5, or 10 years on this compiler or just working in the same building with each other all day are not habits that a community contributor would naturally pick up. We need to fix that.

In summary, your PR has too much churn, make your changes minimal and surgical in the future. Please be respectful of team members, less defensive, and more receptive to feedback. Hold us to being more collaborative up front.

I know normally we don’t like you to open multiple PRs for the same code change because it loses prior feedback, but I think it’s best to roll a do-over on this one. Believe it or not I’ve had to re-implement or re-apply changes for a language feature 4 or 5 times in various branches too. I’m going to close this PR. Don’t throw away any code (especially test cases). If you’re still interested in doing the feature, I’d like to try this again from the top and taking very very small steps together:

• Discussing implementation strategies for the feature.
• The bare minimum amount of code changes to implement the strategy.
• Discussing the test strategy for the feature (enumerating all the new test cases).
• The test code.
• Discussing the LangVersion/feature flag and how to implement it for this feature.
• The bare minimum amount of code to guard this feature on the flag and update test cases.
• Discussing the impact of the feature on the IDE.
• The bare minimum amount of code changes to update the IDE.

I cannot over emphasize the words bare minimum above. I think if we take this in small layered steps we can get this feature in for VB 15.1 with the least amount of friction and frustration on all sides. Given that we’re still new to this and hadn’t set all the expectations or weren’t as collaborative or responsive as we should have there’s no offense taken if you don’t want to start over. I will chalk it up as a failure on me, not you. My concern was that if we were too prescriptive it would take all the fun out of doing the feature and erred on the side of letting you run with this. In hindsight what I was really doing was denying you the team culture and support you needed to be successful. I apologize. That said, is this something you’re still interested in or would you like someone else to pick up the feature?

@AnthonyDGreen Thank you so much for the wonderful feedback, both the good, the bad and the ugly.
Someone had to be first external language feature contributor, I am OK with that. As I see it it has helped the roslyn project community and I to find issues and bugs in the process.

Things to take forward ;-

• Don't treat the code as my code, but as our code. We are the current custodians of the bits.
• Think first. Think again.
• Discuss our results of our thinking.
  • I've been thinking about this, and this my analysis.
  • Gain some feedback some at least someone else, from the Roslyn community.
• Have one true PR feature master branch.
  • Create other branches to prototype on, before pull in to Feature master.
  • Create other branch for "Bug Fixes".
  • Minimal changes (if any) to existing code.

Sad to admit I was expecting a "No Go" on the current PR code for the feature. It way to big as compared to others. Most down to stylistic changes, and feature checks implementations. As proof of concept the PR demonstrates to me, the feature has high potential of get into the Visual Basic language. Even if I'm not the one to implement it. Saying that if it ok with you and the community I'd like to have yet another go at implementing it, as I been on it since inception. *(If another member would like to implement I'm OK with that to)..

@AnthonyDGreen Had a chance to sit back and reflect on this and come the conclusion. That it would be good for the community ( and I ) that if I release development of the feature to someone else. Better to see the feature in a release, rather than waiting for me to eventually implement it. I'll keep that branch publicly available so that whom takes the torch, can reuse the unit tests.

Hey Adam, we're already past the point where I think we'd add this feature to release. I still think it's a good feature and we should work together to get it in an update. We're buttoning up a few things on VB 15 and then we can take another look at implementing this together. Thanks!

@AnthonyDGreen
The redux is in this branch
Yet to port over the unit tests.