[WIP] Extract interfaces for IL symbol definitions #4791

auduchinok · 2018-04-26T09:26:06Z

We'd like to be able to reuse ReSharper caches in FCS. The idea is to integrate into the process of reading assemblies like how it's done in F# cross-project references in FCS itself. This PR adds a shim that can override FCS assembly reading for this purpose and extracts interfaces so other IL symbols representations can be used.

As per initial review by @dsyme implementations for extracted interfaces were removed from il.fsi and thus were hidden.

Add assembly reader shim
Extract interfaces for IL definitions (IModuleDef, IAssemblyManifest, ITypeDef, etc)
Extract interfaces for IL refs/specs (IModuleRef, ITypeRef, ITypeSpec, etc)
performance test (will be done by @dsyme CompilerPerf check)

I'm opening this PR to get feedback while still working on extracting interfaces for other types in IL.

auduchinok · 2018-04-27T09:43:59Z

I was initially asked to put extracting definition interfaces to a single PR. It looks, however, that the PR is really big to make a proper review feasible. Should I open separate PRs instead?

cartermp · 2018-04-27T14:55:09Z

That can be @dsyme and @KevinRansom's call. Personally I think this is fine as-is though

KevinRansom · 2018-04-28T07:31:44Z

It is too big to do a careful review. But it would likely be quite tough to split it apart too. I guess keep on with a single PR, and we will have to be methodical when we do the review.

KevinRansom · 2018-04-28T07:37:28Z

The work so far looks good. On a fairly cursory glance.

auduchinok · 2018-04-28T11:55:21Z

@KevinRansom Most changes are split by commits good enough so it could be relatively easy to split the PR by cherry picking commits and applying some cleanup if it makes things better for review.

KevinRansom · 2018-04-29T05:08:19Z

Give splitting it up a whirl then, if it causes more than a little bother though. We can review it as one lump.

dsyme

I've left a few comments outlining some initial changes to make please - if you feel any of them are strongly wrong please let me know and we can discuss, thanks

dsyme · 2018-05-04T21:30:55Z

src/absil/il.fsi

-      Data: byte[] 
-      Elements: ILAttribElem list}
+/// to ILAttribElem's as best as possible.
+type IAttribute =


Please keep the names as ILAttribute etc.

I guess the point here is to honor .NET guidelines which say interface names should start with I.
This would lead to IILAttribute

Please just use ILAttribute, i.e. the same names as now. Thanks

I guess the point here is to honor .NET guidelines which say interface names should start with I.

Yes, the idea was to follow these guidelines.

@dsyme OK, I'll change the names this way. I do, however, really like idea of using more conventional names. Would you mind returning to the naming discussion at some later point and probably having this stuff renamed in a future PR?

dsyme · 2018-05-04T21:31:20Z

src/absil/il.fsi

-      CustomAttrsStored: ILAttributesStored
-      MetadataIndex: int32 }
-    member CustomAttrs: ILAttributes
+type IParameter =


Likewise, please keep the name as ILParameter etc

dsyme · 2018-05-04T21:31:44Z

src/absil/il.fsi

-      Type: ILType 
-      CustomAttrsStored: ILAttributesStored
-      MetadataIndex: int32  }
+type IReturn =


dsyme · 2018-05-04T21:32:21Z

src/absil/il.fsi

-    member MaxStack: int32
-    member IsZeroInit: bool
+/// IL Method definitions.
+type IMethodDef =


ILMethodDef

dsyme · 2018-05-04T21:36:37Z

src/absil/il.fs

+
+
+[<Sealed>]
+type ILAttribute(method, data, elements) = 


I've requested that the API use the names ILAttribute, partly to completely minimize the code churn and de-emphasize the abstraction being performed here (we do not want to consider arbitrary implementors in this codebase, just implementors that implement contracts very similar to the existing functional-object contracts, e.g. take-no-locks-and-do-no-observable-mutation).

As a result we need to use a different name here. I think it's probably best to use ILAttributeImpl etc.

dsyme · 2018-05-04T21:37:05Z

src/absil/il.fs

-      CustomAttrsStored: ILAttributesStored
-      MetadataIndex: int32  }
-    member x.CustomAttrs = x.CustomAttrsStored.GetCustomAttrs x.MetadataIndex
+type ILParameter


Likewise ILParameterImpl

dsyme · 2018-05-04T21:37:40Z

src/absil/il.fs


-type ILGenericParameterDefs = ILGenericParameterDef list
+[<Sealed>]
+type ILGenericParameterDef


Likewise ILGenericParameterDefImpl

dsyme · 2018-05-04T21:39:32Z

src/absil/il.fs

+                 ?parameters: ILParameters * ?ret: IReturn * ?body: ILLazyMethodBody * ?securityDecls: ILSecurityDecls * ?isEntryPoint:bool * 
+                 ?genericParams: ILGenericParameterDefs * ?customAttrs: IAttributes -> IMethodDef
+
+    abstract WithSpecialName: IMethodDef


I'm not that happy about these being abstract. I think they should just be extension methods defined in terms of With. I'm not interested in trying to reason about implementations that give alternative implementations of these so they shouldn't be abstract

dsyme · 2018-05-04T21:40:34Z

src/absil/il.fs

+        ?newCustomAttrs: IAttributes
+            -> IFieldDef
+
+    abstract WithAccess: ILMemberAccess -> IFieldDef


Likewise I think these should be extension methods rather than abstract please

dsyme · 2018-05-05T12:56:38Z

@auduchinok Could you add the following comment to each interface abstraction? thanks

/// Implementations of this and other similar abstractions in this API must 
/// implement functional object semantics, as if the objects are F# immutable
/// record types, where 
///   1. properties of objects can be accessed concurrently from any thread at any time 
///   2. no locks are required to access properties of objects. 
///   3. the objects are, logically, speaking, immutable 
///   4. the objects do not need to be explicitly disposed

I also would appreciate a confirmation that this is suitable for your purposes in the context of JetBrains: we discussed this over Slack but I'd like to double check.

I do wonder if a simpler abstraction might be easier, where the extension point is simply an FileName --> ILModuleDef function and you populate the existing (non-abstract) IL* types from the JetBrains caches. Why do we need each of the types to be abstract? Will the implementations really be immutable?

Separately, we will certainly need to do a performance test of this: if it results in a perf degradation

auduchinok · 2018-05-05T15:16:00Z

@dsyme Honestly speaking I cannot confirm our implementations are going to satisfy all the four requirements you propose.

Even creating current IL* types objects would require taking a read lock on my side. The good point is we've had a few discussions in Rider/ReSharper teams about it and it looks like we're able to deal with it and we'll manage to allow FCS to take a read lock on any thread.

Our implementations are going to be wrappers for ReSharper caches which are logically mutable, however, these wrappers are going to be logically immutable as long as the assembly write timestamp isn't changed in the file system shim and I'm going to update timestamps on changes that are observable by FCS (which in turn would invalidate the project).

dsyme · 2018-05-08T17:32:16Z

Even creating current IL* types objects would require taking a read lock on my side. The good point is we've had a few discussions in Rider/ReSharper teams about it and it looks like we're able to deal with it and we'll manage to allow FCS to take a read lock on any thread.

I think it's ok if they take a read lock that has nothing to do with FCS.

Our implementations are going to be wrappers for ReSharper caches which are logically mutable, however, these wrappers are going to be logically immutable as long as the assembly write timestamp isn't changed in the file system shim and I'm going to update timestamps on changes that are observable by FCS (which in turn would invalidate the project).

That's probably ok.

dsyme · 2018-05-09T10:33:09Z

@auduchinok What do you think of this?

I do wonder if a simpler abstraction might be easier, where the extension point is simply an FileName --> ILModuleDef function and you populate the existing (non-abstract) IL* types from the JetBrains caches. Why do we need each of the types to be abstract? Will the implementations really be immutable?

i.e. do we really need to make all of these interfaces? Is it that you need to capture context in each object, similar to MetadataIndex for the AbsIL reader?

dsyme · 2018-05-09T10:38:27Z

@dsyme OK, I'll change the names this way. I do, however, really like idea of using more conventional names. Would you mind returning to the naming discussion at some later point and probably having this stuff renamed in a future PR?

From my perspective as compiler contributor I don't want to think of these as interfaces in the normal .NET sense.

If it were possible I'd really rather we didn't open this extension point up at all - it causes chaos when trying to reason about changes to the compiler. If we could truly constrain implementers to implement functional-object semantics then that would be ok. But we can't, so we are opening up a hole in the compiler that means just about any change can be observed as a breaking change by a "bad" implementer.

So I'm still loathe to take this and especially loathe to "advertise" it by sprinkling IFooBar throughout the compiler code....

auduchinok · 2018-05-09T11:14:13Z

@auduchinok What do you think of this?
i.e. do we really need to make all of these interfaces? Is it that you need to capture context in each object, similar to MetadataIndex for the AbsIL reader?

@dsyme Yes, indeed, we need to capture a cache symbol pointer in a similar manner.

7sharp9 · 2018-05-09T13:18:06Z

You can cache fsharp symbols, that’s done in vs4mac and ionide.

…

On Wed, 9 May 2018 at 12:14, Eugene Auduchinok ***@***.***> wrote: @auduchinok <https://github.com/auduchinok> What do you think of this? i.e. do we really need to make all of these interfaces? Is it that you need to capture context in each object, similar to MetadataIndex for the AbsIL reader? @dsyme <https://github.com/dsyme> Yes, indeed, we need to capture a cache symbol pointer in a similar manner. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#4791 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAj7yrbnWYntoE90ydnABYceg6QjdRfDks5tws-HgaJpZM4Tk0rg> .

dsyme · 2018-05-09T13:43:34Z

@dsyme Yes, indeed, we need to capture a cache symbol pointer in a similar manner.

So if we changed MetadataIdx: int32 to MetadataContext: obj or MetadataContext: IntPtr (and passed it through on read as in mkILTypeDefReader, mkILCustomAttrsReader, ) then we could avoid making the majority of them interfaces?

e.g.

member MetadataIndex: int32 
val mkILCustomAttrsReader: (int32 -> ILAttribute[]) -> ILAttributesStored
val mkILSecurityDeclsReader: (int32 -> ILSecurityDecl[]) -> ILSecurityDeclsStored
val mkILTypeDefReader: (int32 -> ILTypeDef) -> ILTypeDefStored

become

[<Struct>]
type MetadataContext =MetadataContext of obj

member MetadataContext: MetadataContext
val mkILCustomAttrsReader: (MetadataContext -> ILAttribute[]) -> ILAttributesStored
val mkILSecurityDeclsReader: (MetadataContext -> ILSecurityDecl[]) -> ILSecurityDeclsStored
val mkILTypeDefReader: (MetadataContext -> ILTypeDef) -> ILTypeDefStored

or

type MetadataContext =
    | NoMetadataContext 
    | MetadataIndex of int32
    | MetadataHandle of obj

member MetadataContext: MetadataContext
val mkILCustomAttrsReader: (MetadataContext -> ILAttribute[]) -> ILAttributesStored
val mkILSecurityDeclsReader: (MetadataContext -> ILSecurityDecl[]) -> ILSecurityDeclsStored
val mkILTypeDefReader: (MetadataContext -> ILTypeDef) -> ILTypeDefStored

or just

type MetadataContext = IntPtr

member MetadataContext: MetadataContext
val mkILCustomAttrsReader: (MetadataContext -> ILAttribute[]) -> ILAttributesStored
val mkILSecurityDeclsReader: (MetadataContext -> ILSecurityDecl[]) -> ILSecurityDeclsStored

etc. I think I would much prefer that solution if it is feasible because it is very non-intrusive. There are downsides to the above but I'm really, really concerned about opening up a large extension point that we would need to maintain.

auduchinok · 2018-09-11T14:21:35Z

@dsyme Yes, after a while I do really like your suggestions and they seem to be the way to go. Let's keep this PR open until I open another one with one of the suggested approaches used.

I think I would still like to propose several changes in these types but these wouldn't be as intrusive as this PR was intended to be. An example of thing I'd like to address is a need to encode and decode custom attributes data — it's better to produce "decoded" values beforehand.

dsyme · 2018-09-19T13:52:33Z

@auduchinok I'll close this since I know you will submit a different PR

auduchinok added 17 commits April 26, 2018 12:06

Add assembly reader shim

1dbd0af

Extract IModuleDef interface

3f62ecc

Make more things public

0f9a542

Extract ITypeDef, ITypeDefs, IPreTypeDef interfaces

4b3dfb0

Extract IAssemblyManifest interface

72d28b9

Extract IMethodDef, IMethodDefs interfaces

967e2aa

Extract IParameter interface

1caec66

Extract IFieldDef, IFieldDefs interfaces

e9b3f52

Extract IEventDef, IEventDefs interfaces

7644192

Extract IPropertyDef, IPropertyDefs interfaces

f66a3fa

Extract IGenericParameterDef interface

edbccbb

Extract IAttribute, IAttributes interfaces

9552f29

Extract IReturn interface

3f70004

Cleanup

7a5bd17

Hide IL types implementations

a62dbe5

Extract IResource, IResources interfaces

250af4c

Cleanup

7924bc9

dsyme requested changes May 4, 2018

View reviewed changes

auduchinok mentioned this pull request May 15, 2018

Assembly reader shim #4915

Merged

auduchinok changed the title ~~Add assembly reader shim, extract interfaces for IL symbols~~ [WIP] Extract interfaces for IL symbol definitions May 17, 2018

dsyme closed this Sep 19, 2018

auduchinok mentioned this pull request Sep 27, 2019

Make ILPreTypeDef interface #7649

Merged

[WIP] Extract interfaces for IL symbol definitions #4791

[WIP] Extract interfaces for IL symbol definitions #4791

Uh oh!

Conversation

auduchinok commented Apr 26, 2018 • edited by dsyme Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

auduchinok commented Apr 27, 2018

Uh oh!

cartermp commented Apr 27, 2018

Uh oh!

KevinRansom commented Apr 28, 2018

Uh oh!

KevinRansom commented Apr 28, 2018

Uh oh!

auduchinok commented Apr 28, 2018

Uh oh!

KevinRansom commented Apr 29, 2018

Uh oh!

dsyme left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsyme commented May 5, 2018

Uh oh!

auduchinok commented May 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dsyme commented May 8, 2018

Uh oh!

dsyme commented May 9, 2018

Uh oh!

dsyme commented May 9, 2018

Uh oh!

auduchinok commented May 9, 2018

Uh oh!

7sharp9 commented May 9, 2018 via email

Uh oh!

dsyme commented May 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

auduchinok commented Sep 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dsyme commented Sep 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

auduchinok commented Apr 26, 2018 •

edited by dsyme

Loading

auduchinok commented May 5, 2018 •

edited

Loading

dsyme commented May 9, 2018 •

edited

Loading

auduchinok commented Sep 11, 2018 •

edited

Loading