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.

Sign up for GitHub

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

Jump to bottom

VB.NET CDate(CStr(Nothing)) should not emit Conversions::ToDate(string) call #2780

Closed
KirillShlenskiy opened this issue May 15, 2015 · 3 comments
Closed

VB.NET CDate(CStr(Nothing)) should not emit Conversions::ToDate(string) call #2780

KirillShlenskiy opened this issue May 15, 2015 · 3 comments
Assignees
@jaredpar
Labels
Area-Compilers Bug Resolution-By Design The behavior reported in the issue matches the current design Tenet-Compatibility Violation of forwards/backwards compatibility in a design-time piece. Verification Not Required
Milestone
1.0 (stable)

Comments

@KirillShlenskiy
Copy link

The below succeeds when compiled with Visual Studio 2013, but fails at runtime when compiled with Visual Studio 2015 RC:

Dim d = CDate(CStr(Nothing))

Edited stack trace:

System.InvalidCastException: Conversion from string "" to type 'Date' is not valid.
  at Microsoft.VisualBasic.CompilerServices.Conversions.ToDate(String Value)

Expected behaviour: conversion is expected to succeed with d = Date.MinValue

Reason: old compiler used to omit call to Conversions.ToDate where it could determine that the argument is always a null string.

IL generated by Roslyn (Visual Studio 2015 RC):

.locals init (
    [0] valuetype [mscorlib]System.DateTime d
)

IL_0000: nop
IL_0001: ldnull
IL_0002: call valuetype [mscorlib]System.DateTime [Microsoft.VisualBasic]Microsoft.VisualBasic.CompilerServices.Conversions::ToDate(string)
IL_0007: stloc.0
IL_0008: ret

IL generated by Visual Studio 2013 VB.NET compiler (note: call to Conversions.ToDate omitted):

.locals init (
    [0] valuetype [mscorlib]System.DateTime d,
    [1] valuetype [mscorlib]System.DateTime VB$t_date$N0
)

IL_0000: nop
IL_0001: ldloca.s d
IL_0003: initobj [mscorlib]System.DateTime
IL_0009: nop
IL_000a: ret

Implications

The new behaviour seems logical, but it breaks existing code in a way which is difficult to detect and fix. There is no way for a developer to easily find all occurrences of the above in a project/solution. The situation is worse with Option Strict Off and when the CDate call is implied (below generates similar IL and succeeds in VS 2013, but errors in 2015):

Option Strict Off
...
Dim d As Date
...
d = vbNullString

In the absence of a compiler warning the occurrences of the above are hard to find and fix in a large codebase.
@KirillShlenskiy KirillShlenskiy changed the title VB.NET CDate(CStr(Nothing)) should not throw VB.NET CDate(CStr(Nothing)) should not emit Conversions::ToDate(string) call May 18, 2015
@jaredpar jaredpar added this to the 1.0 (stable) milestone May 21, 2015
@jaredpar jaredpar added Bug Tenet-Compatibility Violation of forwards/backwards compatibility in a design-time piece. labels May 21, 2015
@jaredpar
Copy link
Member

Thanks for taking the time to report this issue!

@VSadov VSadov assigned AlekseyTs and unassigned VSadov May 22, 2015
@AlekseyTs
Copy link
Contributor

This looks like a bug in VS 2013 compiler. So far, the only scenario I found where an expression CDate(CStr(Nothing)) is emitted as an empty DateTime is when it is used on the right hand side of an assignment. In other contexts, VS 2013 and VS 2015 emit similar code. For example:

return CDate(CStr(Nothing))

Is emitted as

  // Code size       11 (0xb)
  .maxstack  1
  .locals init (valuetype [mscorlib]System.DateTime V_0)
  IL_0000:  ldnull
  IL_0001:  call       valuetype [mscorlib]System.DateTime [Microsoft.VisualBasic]Microsoft.VisualBasic.CompilerServices.Conversions::ToDate(string)
  IL_0006:  stloc.0
  IL_0007:  br.s       IL_0009
  IL_0009:  ldloc.0
  IL_000a:  ret

@AlekseyTs
Copy link
Contributor

Thanks again for taking the time to post this issue. After a lot of internal discussion we decided to not fix this particular issue. The main factors we considered were the following:

  1. This is a bug in the constant optimization phase of the old compiler. It was never the intended behavior of the language.
  2. This bug does not occur for every use of CDate(CStr(Nothing)) but instead for an undetermined subset of expressions. This makes replicating the bug in Roslyn a moderately expensive item. It also carries a significant risk as it is difficult to identify all possible scenarios where this bug occurred.
  3. In searching our internal code bases at Microsoft and the internet at large we couldn’t find any uses of this pattern. It is possible we missed something (and if so please do point us towards it).
  4. This bug makes it more difficult to construct IDE tooling such as refactoring. Such tools would necessarily need to be made aware of the inconsistent behavior of this expression. This would be a burdensome cost.

Of the above the second and third bullet points were the most influential. At this point in the release we need to have both a compelling case for fixing bugs and high confidence we haven’t regressed other scenarios. This bug doesn’t meet that criteria at this time.

@AlekseyTs AlekseyTs added Resolution-By Design The behavior reported in the issue matches the current design and removed 3 - Working labels Jun 2, 2015
@AlekseyTs AlekseyTs removed their assignment Jun 2, 2015
@jrmoreno1 jrmoreno1 mentioned this issue Mar 2, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jaredpar jaredpar

Labels
Area-Compilers Bug Resolution-By Design The behavior reported in the issue matches the current design Tenet-Compatibility Violation of forwards/backwards compatibility in a design-time piece. Verification Not Required
Projects
None yet
Milestone
1.0 (stable)
Development

No branches or pull requests
5 participants
@jaredpar @Pilchie @VSadov @AlekseyTs @KirillShlenskiy