Skip to content
This repository has been archived by the owner on Aug 24, 2022. It is now read-only.

- TypeUtil.IsStructImmutable() added, [JSImmutable] works with readonly/structs and more #516

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

dzeitlin
Copy link

@dzeitlin dzeitlin commented Jun 2, 2014

  • Immutable structs with member methods (no MemberwiseClone() anymore)
  • Exception.GetType() implemented
  • JSIL.Array.FindIndex()/Find()/ForEach()/ConvertAll() implemented via proxy (support typed arrays)
  • Float and Double +/-Infinity added via proxy
  • Builtins.IsFalsy/IsTruthy() explicit cast to object in parameter (othewise it's dynamic call)
  • Configuration.EmitAttributes list introduced (minimizes .js output substantially by alsmot 25%),
    mscorlib.js droped from 9400 KB to 7140 KB
    use in jsilconfig like:
    "CodeGenerator": {
    "EmitAttributes": [
    "NUnit.Framework"
    ]
    }
  • JSIL.Meta.dll made strong named

…ly/immutable structs with member methods (no MemberwiseClone() anymore)

- Exception.GetType() implemented
- JSIL.Array.FindIndex()/Find()/ForEach()/ConvertAll() implemented via proxy (support typed arrays)
- Builtins.IsFalsy/IsTruthy() explicit case to object in parameter
- Configuration.EmitAttributes list introduced (minimizes .js output substantially)
- JSIL.Meta.dll made strong named
- Float and Double +/-Infinity added via proxy
@@ -539,6 +539,13 @@ JSIL.ImplementExternals(
}
);

$.Method({ Static: false, Public: true }, "GetType",
new JSIL.MethodSignature(mscorlib.TypeRef("System.Type"), []),
Copy link
Member

Choose a reason for hiding this comment

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

The recently introduced MethodSignature.Return(T) is appropriate for signatures like this when writing new code.

Copy link
Author

Choose a reason for hiding this comment

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

How should I use it?

Copy link
Member

Choose a reason for hiding this comment

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

MethodSignature.Return(T) === new JSIL.MethodSignature(T, [], []), but the instance is cached so memory usage is lower and invocation thunks are shared

@kg
Copy link
Member

kg commented Jun 2, 2014

Hi,

This looks great, thanks for the contributions.

I'm not willing to merge a strong name because of the various problems with strong names (especially given that I do not do binary/source compatibility versioning of JSIL assemblies). Given the fact that the actual contents of JSIL.Meta do not matter (just the attributes), I think it is not useful for it to have a strong name. If you have some further thoughts on this to explain why you think it's a worthwhile change, I'd be glad to hear them.

@dzeitlin
Copy link
Author

dzeitlin commented Jun 2, 2014

Our project (WebGL engine) is split between assemblies (one of the assemblies is for example UnitTests).
We use [assembly: InternalsVisibleTo("....")] attrubute.
The only way to do this is to make all strong name, including the referenced JSIL.Meta.dll.
SpecificVersion=False can help to the problem you mention

@markusjohnsson
Copy link
Contributor

Are you sure you can't use InternalsVisibleTo without strong naming? Pretty sure that you can.

…() methods

- AssemblyTranslator TimeSpan.MaxValue debug section removed, it's still bug, somehow TimeSpan.MaxValue/MinValue are not initialized
@dzeitlin
Copy link
Author

dzeitlin commented Jun 2, 2014

You need to sign both assemblies, because effectively both assemblies reference each other.
http://stackoverflow.com/questions/1123683/how-to-declare-a-friend-assembly

@markusjohnsson
Copy link
Contributor

Ah, yes. Either both must be signed or both must be unsigned:
http://msdn.microsoft.com/en-us/library/system.runtime.compilerservices.internalsvisibletoattribute.aspx

@dzeitlin
Copy link
Author

dzeitlin commented Jun 2, 2014

Yes you right, InternalsVisibleTo() both can be NOT signed, it worked.
I changed it here, but still as we develop SDK we deploy only signed assemblies out.
We will overcome it by adding last step of ILmerge with strong signing / resigning I will say.

@iskiselev
Copy link
Member

Also, we have one more problem with JSIL.Meta now - it is not PCL assembly, so we cannot use it inside PCL projects (but really it will be easy enough to do it PCL). On other hand, it should be possible to insert sources of JSIL meta attributes in any users assembly, as JSIL checks them only by name (same way as you can insert Resharper meta attributes insise your own assembly to exclude references to Jetbrains assembly).

@iskiselev
Copy link
Member

One more question - maybe it will be better use meta attributes and proxies for specifying .Net attributes that should be dropped from JS output instead of using .jsilconfig for it. For sure, we should exclude attributes that marked with JSIgnore.

@kg
Copy link
Member

kg commented Jun 2, 2014

Moving JSIL.Meta to PCL makes a lot of sense, we should definitely do that.

I think I agree that filtering attributes should be done with JSIgnore as a baseline, that way any code that references a filtered attribute will generate 'reference to ignored type' instead of just mysteriously failing to find the attributes it's looking for.

If you need strong names because you're deploying a strong named assembly, that's understandable, but in that case I would suggest either signing your build of JSIL.Meta (quite inconvenient, of course), or directly including the .cs files for the attributes you use (as @iskiselev pointed out, JSILc only checks by full name, not by assembly, so this will work fine.)

@dzeitlin
Copy link
Author

dzeitlin commented Jun 3, 2014

I agree [JSIgnore] on attributes must drop it from JS output.
But from my point using [JSIgnore] simply not feasible, only in mscorlib you have I think >30 diffrent attributes (write for all of them proxies??), look:

  • System.Runtime.InteropServices.ComVisibleAttribute (there are many COM irrelevant attributes)
  • System.Runtime.InteropServices.ClassInterfaceAttribute
  • System.Runtime.InteropServices.ComDefaultInterfaceAttribute
  • System.Runtime.InteropServices.GuidAttribute
  • System.Runtime.TargetedPatchingOptOutAttribute
  • System.CLSCompliantAttribute
  • __DynamicallyInvokableAttribute
  • System.Runtime.CompilerServices.FriendAccessAllowedAttribute
  • System.Security.SecurityCriticalAttribute (bunch of attribute here too)
  • System.Security.SecuritySafeCriticalAttribute
  • System.Security.SuppressUnmanagedCodeSecurityAttribute
  • System.Diagnostics.ConditionalAttribute
  • System.AttributeUsageAttribute
  • DebuggerStepThrough and many other DebuggerAttributes
  • System.Runtime.Serialization.OptionalFieldAttribute

It will be nice to have an option to say which attributes to emit via REGEX (be default ALL of course).
Again my idea was to minimize the JS output, by 25% in case of mscorlib.dll.
It is also very simple to implement and use as I did (10 lines of code).
In our case we eliminate all attributes, except NUnit ones, since we have implemented small NUnit runner which is also compiled to JS and tests the JS assembly via reflection.

@dzeitlin
Copy link
Author

dzeitlin commented Jun 3, 2014

To clarify, my code only removed the .Attribute() reference, not the attributes definitions themselves and this results in reducing mscorlib.js substantially.

@kg
Copy link
Member

kg commented Jun 3, 2014

JSProxy accepts a list of class names (check the overloads), so you'd just
need one proxy class with a JSIgnore attribute.

On Tue, Jun 3, 2014 at 4:30 AM, Daniel [email protected] wrote:

To clarify, my code only removed the .Attribute() reference, not the
attributes definitions themselves and this results in reducing mscorlib.js
substantially.


Reply to this email directly or view it on GitHub
#516 (comment).

@iskiselev
Copy link
Member

@dzeitlin, I really can't imagine why may I want don't include attribute in code, but still preserve it's definition. Even in cases, where I may want use this option - I need much more feature-rich config with not only options to preserve/exclude some attributes, but also with option to point members on which I should exclude such attributes.
On other side, it looks like interesting option to have an ability of applying any meta attribute with Regex in *.jsilconfig. If @kg doesn't mind against it - we could implement such option.

…type, very useful)

- Don't define (DeclaringType) on skipped/ignored attributes
- TypeUtil.IsStructImmutable(TypeReference type) is more robust, all fields are 'readonly' -> struct is immutable, (much faster than: typeInfo = TypeInfo.GetTypeInformation(type) + typeInfo.IsImmutable)
@dzeitlin
Copy link
Author

dzeitlin commented Jun 4, 2014

@iskiselev, @kg User defined ignored Attributes are skipped as well and not defined (saves another 4% from mscrolib.js output, now it's only 7048KB, originally was 9400 KB).

Also I very recommend to introduce "T Verbatim.Expression < T >(string javascript)" see my commit, it simply saves the return cast in many cases, for example OnKeyUp(dynamic e):
int keyCode = Verbatim.Expression< int >("e.keyCode"); // no cast to int here

@dzeitlin
Copy link
Author

dzeitlin commented Jun 4, 2014

Also I'm going to introduce [JSChangeEnumToNumber] optional attribute on enums. Such enum will be converted to JS number, enum will be eliminated. Very usefull for example in GLenum case, has hundreds of enum values defined.

  1. enum will be not defined
  2. no casts will be needed
  3. enum.value will be replaced with value or with (value1 | value2 | ..) evaluated!
  4. option to insert enum.value as a comment for easy debugging, var kind = /* Kinds.Five */ 5;

I already implemented and I will commit it tomorrow. Any ideas?

@kg
Copy link
Member

kg commented Jun 4, 2014

I'm not sure I'm happy with the idea of dropping type system info from enums. We already drop too much info about numbers, so that will only make it worse.

Have you explored the edge cases where that breaks down? How bad are they?

- int? CodeGeneratorConfiguration.ChangeEnumToNumber added, values are: 0 - disabled, 1 - with attribute only, 2 - all enums
- TypeUtil.IsEnum() enhanced to report about enums to number change
- Enums changed to number (Int32) implemented: generic parameter, default(Tenum), $Cast()
@dzeitlin
Copy link
Author

dzeitlin commented Jun 8, 2014

As I mentioned I successfully implemented the [JSChangeEnumToNumber] attribute.
Also I added CodeGeneratorConfiguration.ChangeEnumToNumber: int? with it you can disable or force all enums to be converted to JS number type (in so called "release" JSILc run).
Benefits from this conversion are:

  1. performance 2) code minimization 3) some obfuscation

I handled and implemented:

  • direct enum value calculation with no casts at all
  • no more casts (int->enum, enum->int, enumA -> enumB)
  • default(T_Enum), and T_enum as generic parameter
  • [Flags] are automatically using 0xHEX convention
  • Comments are embedded, such as: var kind = /* Kinds.Five */ 5;
  • declare type MakeEnum removed

The improvement is quite straight-forward, I enhanced the already known TypeUtil.IsEnum(type) : bool method, to return state of enum (keep it as is or convert to number).
All these is of course optional, and disabled by default.

@kg
Copy link
Member

kg commented Jun 10, 2014

So I am ready to merge some but not all of the commits in here. We have two options:
One, you can split the individual change units in here (like Verbatim.Expression<T>, ChangeEnumToNumber, etc) out into PRs, so we can track them individually. That makes it possible to discuss each one and track them independently.
Two, I pull commits I'm okay with out of this PR and merge them, and the rest of this PR rots forever. If I do that, you're welcome to pull the unmerged commits out and make a PR later once they're cleaned up.

Multiple commits are fine, and multiple commits in related areas are usually fine, this is just too much disjoint stuff for me to merge unless I'm okay with all of it (and for me, ChangeEnumToNumber and the strong name and the list of attributes in the config are all things I'm not okay with merging into trunk, at least as-is)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants