Skip to content

split TypeChecker.fs#10317

Merged
dsyme merged 6 commits into
dotnet:mainfrom
dsyme:tcsplit
Oct 22, 2020
Merged

split TypeChecker.fs#10317
dsyme merged 6 commits into
dotnet:mainfrom
dsyme:tcsplit

Conversation

@dsyme

@dsyme dsyme commented Oct 22, 2020

Copy link
Copy Markdown
Contributor

This is cleanup to split TypeChecker.fs into two chunks of 12,000 and 6,000 lines.

The chunks are

  • CheckExprs.fs - check syntactic types and expressions, and many aspects of members and recursion (since object expressions include members)

  • CheckDecls.fs - check type declarations, modules, namespaces, signatures, files

This necessarily involves opening up some of the internals of TypeChecker.fs in the signature of CheckExprs.fsi but I think that's ok. More is opened up than I would like (and we may be able to reduce that in the future) but it's not too bad.

We can do further splitting at a later point, e.g. split out CheckComputationExprs.fs, CheckObjectExprs.fs, TcFileState.fs, TcEnv.fs and so on (if that proves the right split)

@forki

forki commented Oct 22, 2020

Copy link
Copy Markdown
Contributor

but what will we use as benchmark after this? :P

@dsyme

dsyme commented Oct 22, 2020

Copy link
Copy Markdown
Contributor Author

but what will we use as benchmark after this? :P

LOL I was just adjusting those benchmarks and thinking "oh wow, this will speed them up"

@dsyme

dsyme commented Oct 22, 2020

Copy link
Copy Markdown
Contributor Author

BTW it feels like a good time to do this split, as AFAIK there are relatively few outstanding changes to TypeChecker.fs in proposed features or PRs

@dsyme

dsyme commented Oct 22, 2020

Copy link
Copy Markdown
Contributor Author

Hmmm transient failure on Mac

2020-10-22T13:17:40.0576150Z [xUnit.net 00:00:26.55]     FSharp.Core.UnitTests.Control.MailboxProcessorType.Scan handles cancellation token [FAIL]
2020-10-22T13:17:40.0607290Z   X FSharp.Core.UnitTests.Control.MailboxProcessorType.Scan handles cancellation token [5s 156ms]
2020-10-22T13:17:40.0608500Z   Error Message:
2020-10-22T13:17:40.0609440Z    System.NullReferenceException : Object reference not set to an instance of an object.
2020-10-22T13:17:40.0610320Z   Stack Trace:
2020-10-22T13:17:40.0611230Z      at FSharp.Core.UnitTests.Control.MailboxProcessorType.Scan handles cancellation token() in /Users/runner/work/1/s/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/MailboxProcessorType.fs:line 168

@dsyme

dsyme commented Oct 22, 2020

Copy link
Copy Markdown
Contributor Author

@cartermp I'm loving the fact that the compiler guide is in the tree and we can search and replace to help update it

@KevinRansom KevinRansom left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Don, this is good.

@cartermp cartermp left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My approval is one of happiness and sadness.

Happiness that this is finally getting split up into some logical chunks!

Sadness that, like @forki, I no longer have this ridiculous amazing benchmark for various tooling experience smoke tests and benchmarks.

@baronfel

Copy link
Copy Markdown
Member

I'm just amazed that now GitHub will actually render the content of the files, so we can deep-link directly to relevant code when showing folks around.

@dsyme

dsyme commented Oct 22, 2020

Copy link
Copy Markdown
Contributor Author

Yes it's the end of an era. But at least there are still 230 character lines in the files to enjoy.

@dsyme dsyme merged commit 0b69e41 into dotnet:main Oct 22, 2020
errorR(Error(FSComp.SR.tcInvalidNamespaceModuleTypeUnionName(), id.idRange))

let CheckDuplicates (idf: _ -> Ident) k elems =
elems |> List.iteri (fun i uc1 ->

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest a small optimization here:

let CheckDuplicates (idf: _ -> Ident) k elems = 
    let ids = elems |> List.mapi (fun i uc -> i, idf uc)
    ids
        |> List.iter (fun (i, id1) ->
            ids
                |> List.iter (fun (j, id2) ->
                    if j > i && id1.idText = id2.idText then 
                        errorR (Duplicate(k, id1.idText, id1.idRange))))
    elems

because is computing idf for all elements too many times n * (n + 1) instead of just n

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you please send it as new PR. I'd also suggest you unroll it as nested for loops .

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The suggested code will look like this:

let CheckDuplicates (idf: _ -> Ident) k elems = 
    let ids = elems |> List.mapi (fun i uc -> i, idf uc)
    for (i, id1) in ids do
        for (j, id2) in ids do
            if j > i && id1.idText = id2.idText then 
                errorR (Duplicate(k, id1.idText, id1.idRange))
    elems

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@forki The PR is #10325

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.

7 participants