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

Fix 457 - add ones complement operator with backwards compat #458

Closed
wants to merge 3 commits into from

Conversation

@dsyme
Copy link
Collaborator

commented May 19, 2015

This is a tentative fix for #457

Some testing still needs to be added:

  • test use of ~~~x for the very rare case of a user-defined type that supports the op_LogicalNot operator when compiling against FSharp.Core 4.3.1.0 (when moving to FSharp.Core 4.4.0.0 and F# 4.0, this code will need to be changed to !!!x)

The F# Language Spec will need to be updated to say !!! is for op_LogicalNot and ~~~ is for op_OnesComplement.

In theory this is a breaking change in some situations - in particular "let (~~~) x = x" now compiles as a function called "op_OnesComplement" instead of "op_LogicalNot". However FSharp.Core remains fully backwards binary compatible because both functions are now present. I think this is ok given the situation.

@latkin

This comment has been minimized.

Copy link
Contributor

commented May 19, 2015

Yeah seems like op_LogicalNot is pretty rare so I doubt we will break a lot of people. No usages in mscorlib. Full-GitHub search returns only testcode for language-type platforms that I can see (plus our incorrect usage 😄)

Will this play nicely with quotations when used cross-version? e.g. https://code.google.com/p/unquote/issues/detail?id=49&can=1&q=op_LogicalNot

@dsyme

This comment has been minimized.

Copy link
Collaborator Author

commented May 19, 2015

It should all work across version. So <@ ~~~1 @> compiled with F# 3.1 and evaluated with F# 4.0 (or with an F# 3.x component with a binding redirect to FSharp.Core 4.4.0.0) should work correctly. That testing would be good to add (basically cross-version execution for tests/fsharp/core/queriesLeafExpressionConvert/test.fsx I believe)

@@ -531,6 +532,7 @@ module LeafExpressionConverter =
| LessQ (_, _,[x1;x2]) -> transBinOp env false x1 x2 false Expression.LessThan
| LessEqQ (_, _,[x1;x2]) -> transBinOp env false x1 x2 false Expression.LessThanOrEqual
| NotQ (_, _, [x1]) -> Expression.Not(ConvExprToLinqInContext env x1) |> asExpr
| LogicalNotQ (_, _, [x1]) -> Expression.Not(ConvExprToLinqInContext env x1) |> asExpr

This comment has been minimized.

Copy link
@latkin

latkin May 19, 2015

Contributor

Wow, NotQ, LogicalNotQ, and BitwiseNotQ all map to Expression.Not... That guy will pick if he represents logical or bitwise operation based on operand type and availability of the corresponding op_* guys: source. The docs only mention the bitwise operation.

Sheesh, queries are confusing...

This comment has been minimized.

Copy link
@dsyme

dsyme May 19, 2015

Author Collaborator

Thanks for checking!

@latkin

This comment has been minimized.

Copy link
Contributor

commented May 19, 2015

Blah, this indeed has some nasty cross-version problems.

Scenario 1

printfn "%A" <| (~~~ 2)
  • Build as exe w/ 4.0 compiler, targeting 3.1
    • Fails at runtime System.NotSupportedException: Specified method is not supported.

Scenario 2

module Test
let q = <@ ~~~ 1 @>
  • Builds ok w/ 3.1 compiler targeting 3.1
  • Builds ok w/ 4.0 compiler targeting 4.0
  • Fails w/ 4.0 compiler targeting 3.1
    • error FS0458: Quotations cannot contain expressions that make member constraint calls, or uses of operators that implicitly resolve to a member constraint call

Scenario 3

module Test

let (!!!) (b : bool) = 
    printfn "in custom bool !!! op"
    b
  • Build w/ 3.1, consume w/ 4.0
    • !!! now maps to built-in op_LogicalNot, not to the custom guy
    • To access custom operator, need to use op_BangBangBang
    • Test.( !!! ) ;; fails - The value, constructor, namespace or type '!!!' is not defined
  • Build w/ 4.0 targeting 3.1, consume w/ 3.1
    • Custom operator is incorrectly interpreted as ~~~

Scenario 4

module Test

let (~~~) (b : bool) = 
    printfn "in custom bool ~~~ op"
    b
  • Build w/ 3.1, consume w/ 4.0
    • Custom operator is incorrectly interpreted as !!!
  • Build w/ 4.0 targeting 3.1, consume w/ 3.1
    • Custom operator can only be accessed via op_OnesComplement
    • ~~~ still maps to built-in op_LogicalNot
@dsyme

This comment has been minimized.

Copy link
Collaborator Author

commented May 19, 2015

Urgh. Good work. Tricky problems.

I'll take a look tomorrow. It's possible most of the "4.0 targeting 3.1" problems go away if we vary the name mapping used based on a 4.3.x.0 v. 4.4.0.0 library target.

@@ -405,13 +405,14 @@ type public TcGlobals =
bitwise_or_vref : ValRef;
bitwise_and_vref : ValRef;
bitwise_xor_vref : ValRef;
bitwise_unary_not_vref : ValRef;
bitwise_unary_logical_not_vref : ValRef;

This comment has been minimized.

Copy link
@PatrickMcDonald

PatrickMcDonald May 20, 2015

Contributor

Is the naming of this correct? I always thought bitwise and logical were mutually exclusive.

@dsyme

This comment has been minimized.

Copy link
Collaborator Author

commented May 20, 2015

This is VNext at this stage (hopefully first OOB, if we can find a way to make it fully backwards compat, otherwise next major language revision I suppose)

@dsyme dsyme added the V.Next label May 20, 2015

@latkin

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2015

This has been idle for a couple months and not yet clear how to fix this without a surface area change and still maintain cross-version compat. Thus closing out for now.

@latkin latkin closed this Aug 4, 2015

@latkin latkin removed the V.Next label Aug 4, 2015

@dsyme

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 4, 2015

@latkin I'm not sure "idle for a couple of months" is justification when the repo has effectively been closed for business for bug fixing for at least a couple of months :)

Lets just prompt the owners to reopen the PRs against the appropriate branch?

@latkin

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2015

@dsyme I'm only closing idle PRs which have unresolved feedback and/or known issues, not those which are thought to be fully ready but "waiting in the queue."

@dsyme

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 4, 2015

My fear for this particular one is that if it's closed - apart from bug #457 - then I'll just keep punting it. "Out of sight out of mind, I tried once and it didn't stick so I gave up" etc.

We need to have a plan in place to fix this in the language design. I suppose tracking it on fslang.uservoice.com would make sense since it's effectively baked in as part of the 4.0 design at the moment.

@dsyme

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 4, 2015

Given the failure of the above approach I suppose a way forward would be

  1. add new complement and logicalNot operators that work as intended. See also this workaround
  2. add a compiler warning when ~~~ statically resolves to a non-primitive op_LogicalNot operator, or doesn't resolve statically at all (in let inline ... code). The warning would direct the user to use complement or logicalNot explicitly instead.
  3. In some far off future revision of F# change ~~~ to resolve to op_OnesCoplement and consider adding the symbolic !!! operator for op_LogicalNot.

Does that sound reasonable? If the approach is sound then steps 1-2 require updating FSharp.Core of course.

@dsyme

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 4, 2015

I've added an entry on fslang.uservoice.com to track this: http://fslang.uservoice.com/forums/245727-f-language/suggestions/9168976-add-complement-and-logicalnot-operators-to-res, documenting the above plan

@latkin

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2015

That sounds reasonable to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.