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

Ignore some base constructor calls #2298

Merged
merged 1 commit into from
Nov 27, 2020

Conversation

ncave
Copy link
Collaborator

@ncave ncave commented Nov 25, 2020

  • Ignore some base constructor calls (e.g. for BCL types):
type MyList<'T>() =
    inherit ResizeArray<'T>()
    member x.Length = x.Count

@alfonsogarciacaro
Copy link
Member

alfonsogarciacaro commented Nov 26, 2020

I guess this is related to #2154. Is it a good solution to just ignore the base constructor call? Or should we try to inherit from JS Array instead?

@ncave
Copy link
Collaborator Author

ncave commented Nov 26, 2020

@alfonsogarciacaro No, it's not related, I just noticed that some base constructor calls are in the form of Value instead of a Call. Not sure if that's a bug in FCS or is intended, so this PR just provides an opportunity to replace the hard error with a warning.
Technically in this case, yes, the base type should be JS array, but there are other base BCL types we don't have a clear JS replacement for, so I think it's ok to just warn. Inheriting from BCL types is a fringe practice anyway, should be composition instead of inheritance most of the time, unless there is a replacement for the base type already, like in the case of Dictionary.

@alfonsogarciacaro
Copy link
Member

Yes, some calls to the BCL have an unexpected representation in the AST, for example DateTime.MinValue appears as BasicPatterns.ILFieldGet. We do have a function to get the reference/constructor of a native JS type, which we could try to use later. But for now, I guess the warning message is enough to let the user know something is going funny. Thanks!

let tryEntityRef (com: Compiler) entFullName =
match entFullName with
| BuiltinDefinition BclDateTime
| BuiltinDefinition BclDateTimeOffset -> makeIdentExpr "Date" |> Some
| BuiltinDefinition BclTimer -> makeImportLib com Any "default" "Timer" |> Some
| BuiltinDefinition BclInt64
| BuiltinDefinition BclUInt64 -> makeImportLib com Any "default" "Long" |> Some
| BuiltinDefinition BclDecimal -> makeImportLib com Any "default" "Decimal" |> Some
| BuiltinDefinition BclBigInt -> makeImportLib com Any "BigInteger" "BigInt/z" |> Some
| BuiltinDefinition(FSharpReference _) -> makeImportLib com Any "FSharpRef" "Types" |> Some
| BuiltinDefinition(FSharpResult _) -> makeImportLib com Any "FSharpResult$2" "Choice" |> Some
| BuiltinDefinition(FSharpChoice _) -> makeImportLib com Any "FSharpChoice$2" "Choice" |> Some
// | BuiltinDefinition BclGuid -> jsTypeof "string" expr
// | BuiltinDefinition BclTimeSpan -> jsTypeof "number" expr
// | BuiltinDefinition BclHashSet _ -> fail "MutableSet" // TODO:
// | BuiltinDefinition BclDictionary _ -> fail "MutableMap" // TODO:
// | BuiltinDefinition BclKeyValuePair _ -> fail "KeyValuePair" // TODO:
// | BuiltinDefinition FSharpSet _ -> fail "Set" // TODO:
// | BuiltinDefinition FSharpMap _ -> fail "Map" // TODO:
| Types.matchFail -> makeImportLib com Any "MatchFailureException" "Types" |> Some
| Types.exception_ -> makeIdentExpr "Error" |> Some
| _ -> None
let tryJsConstructor com (ent: Entity) =
if FSharp2Fable.Util.isReplacementCandidate ent
then tryEntityRef com ent.FullName
else FSharp2Fable.Util.tryEntityRefMaybeGlobalOrImported com ent

@alfonsogarciacaro alfonsogarciacaro merged commit e3f3cbe into fable-compiler:nagareyama Nov 27, 2020
@ncave ncave deleted the next1 branch November 28, 2020 21:42
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.

2 participants