Some fixes to run arg_scanner on diaspora specs. #7

vladimir-koshelev · 2017-10-16T13:42:21Z

No description provided.

valich · 2017-10-26T17:16:59Z

Review: https://upsource.jetbrains.com/RUBI/review/RUBI-CR-33

valich · 2017-10-26T18:30:41Z

arg_scanner/lib/arg_scanner/type_tracker.rb

@@ -1,6 +1,7 @@
 require 'set'
 require 'socket'
 require 'singleton'
+require 'thread'



valich · 2017-10-27T10:39:53Z

contract-creator/src/org/jetbrains/ruby/runtime/signature/server/SignatureServer.kt

-                val currRTuple = ben(jsonTome) { RTupleBuilder.fromJson(jsonString) }
-
-                if (currRTuple?.methodInfo?.classInfo?.classFQN?.startsWith("#<") == true) {
+        try {


Several nested try-while-try-etc are very difficult to understand. We have to refactor this into several methods.

Also: what kind of exception may occur which requires finally block in the end? I do not see any returns from the above code.a

valich · 2017-10-27T10:41:34Z

contract-creator/src/org/jetbrains/ruby/runtime/signature/server/SignatureServer.kt

@@ -125,11 +147,22 @@ object SignatureServer {
            try {
                val br = BufferedReader(InputStreamReader(socket.getInputStream()))

+                var iter = 0L
+                var millis = System.currentTimeMillis()


These fields and corresponding logic are just logging/debugging goodness. I think it should be encapsulated into some logger class or something; in other case it's difficult to understand where business logic starts and debugging info does.

valich · 2017-10-27T10:42:22Z

contract-creator/src/org/jetbrains/ruby/runtime/signature/server/SignatureServer.kt

                    val currString = ben(readTime) { br.readLine() }
                            ?: break
-                    if (queue.size > queue.remainingCapacity()) {
-                        LOGGER.info("Queue capacity is low")
+                    val remainingCapacity = queue.remainingCapacity()


Not sure this extraction makes much sense

valich · 2017-10-27T10:42:57Z

contract-creator/src/org/jetbrains/ruby/runtime/signature/server/SignatureServer.kt

-        SchemaUtils.create(GemInfoTable, ClassInfoTable, MethodInfoTable, SignatureTable)
-        transaction.commit()
-        runServer()
+        Database.connect("jdbc:mysql://localhost:3306/" + "ruby_type_contracts" + "?serverTimezone=UTC&nullNamePatternMatchesAll=true&useSSL=false",


DatabaseProvider.connect()?

valich · 2017-10-27T10:48:37Z

ide-plugin/src/org/jetbrains/plugins/ruby/ruby/codeInsight/types/RubyStatTypeProviderImpl.java

+        final Set<String> returnTypes = contract.Companion.getAllReturnTypes(contract);
+        final RType trueType = RBooleanType.getTrueType(project);
+        final RType falseType = RBooleanType.getFalseType(project);
+        final RType booleanType = RTypeUtil.union(trueType, falseType);


org.jetbrains.plugins.ruby.ruby.codeInsight.types.RTypeFactory#createBoolType

valich · 2017-10-27T10:57:56Z

ide-plugin/src/org/jetbrains/plugins/ruby/ruby/codeInsight/types/RubyStatTypeProviderImpl.java

@@ -193,42 +250,62 @@ public RType createTypeByCallAndArgs(@NotNull final RExpression call, @NotNull f

            if ((arg instanceof RAssoc) ^ (paramInfos.get(i1).getModifier() == ParameterInfo.Type.KEY ||
                    paramInfos.get(i1).getModifier() == ParameterInfo.Type.KEYREQ)) {
-                Logger.getInstance(RubyStatTypeProviderImpl.class).error("wtf");
+                return REmptyType.INSTANCE;


Is there a sample of test on which this line fails?

valich · 2017-10-27T10:59:02Z

ide-plugin/src/org/jetbrains/plugins/ruby/ruby/codeInsight/types/RubyStatTypeProviderImpl.java

+        RType returnVal = returnTypes.stream()
+                .map(name -> RTypeFactory.createTypeByFQN(call.getProject(), name))
+                .reduce(REmptyType.INSTANCE, RTypeUtil::union);
+        if (returnVal != REmptyType.INSTANCE) {


Please extract debugging facilities.

valich · 2017-10-27T10:59:22Z

ide-plugin/src/org/jetbrains/plugins/ruby/ruby/codeInsight/types/RubyStatTypeProviderImpl.java

@@ -307,7 +384,7 @@ private static MethodInfo findMethodInfo(@NotNull final PsiElement callElement)
            arg = ((RAssoc) arg).getValue();
        }

-        final Set<String> argTypeNames = new HashSet<>();
+        final Set<String> argTypeNames = new LinkedHashSet<>();


Why is order imporant here?

You can sort stuff in debug messages; now you add memory and (little) cpu overhead for common code

valich · 2017-10-27T11:03:18Z

...java/org/jetbrains/ruby/codeInsight/types/signature/serialization/MethodInfoSerialization.kt

+        }
+        stream.writeInt(PROTOCOL_VERSION)
+        stream.writeInt(gemInfo2Id.size)
+        var iter = 0


Not sure this assertion is very helpful

I don't like the looks. The method is 40 lines long so every such variable complicates the state for the reader.

valich · 2017-10-30T11:25:20Z

contract-creator/src/org/jetbrains/ruby/runtime/signature/server/SignatureServer.kt

@@ -70,36 +84,44 @@ object SignatureServer {



line 76

It was used in RubyStatTypeProviderImpl

valich · 2017-10-30T15:12:59Z

contract-creator/src/org/jetbrains/ruby/runtime/signature/server/SignatureServer.kt

@@ -70,36 +84,44 @@ object SignatureServer {



line 76

I suggest addressing that in another PR in order to increase the possiblity of that one be merged :)

valich · 2017-11-01T11:39:44Z

contract-creator/src/org/jetbrains/ruby/runtime/signature/server/SignatureServer.kt

@@ -70,36 +84,44 @@ object SignatureServer {



line 76

Next time, yes, please.

valich · 2017-11-01T11:41:15Z

arg_scanner/lib/arg_scanner/type_tracker.rb

@@ -13,7 +56,9 @@ class TypeTracker
    def initialize
      @cache = Set.new
      @socket = TCPSocket.new('127.0.0.1', 7777)
-
+      @mutex = Mutex.new


I'd suggest instantiating it only on some condition (debugging env variable, etc)

btw I think @perf_mon = TTPM.new if @enable_debug will do the same :)
But we'll do it LATER

valich · 2017-11-01T11:42:34Z

contract-creator/src/org/jetbrains/ruby/runtime/signature/server/SignatureServer.kt

@@ -142,7 +165,31 @@ object SignatureServer {
                } catch (e: IOException) {
                    LOGGER.severe("Can't close a socket")
                }
+                onExit(handlerNumber)
+            }
+        }


valich · 2017-11-01T11:48:25Z

ruby-call-signature/src/test/java/org/jetbrains/ruby/codeInsight/types/signature/MergeTest.kt

-        val args5 = listOf("Int1", "Int2", "Int3")
+    private fun generateComplicatedContract() : RSignatureContract {
+        val args1 = listOf("a1", "c2", "a3", "a4")
+        val args2 = listOf("a1", "b2", "a3", "a4")


Why have you changed existing tests? You might add new ones instead

valich · 2017-11-01T11:48:51Z

ruby-call-signature/src/test/java/org/jetbrains/ruby/codeInsight/types/signature/MergeTest.kt


-        val testArgs1 = listOf("Int1", "Int2", "Int3")
-        val testArgs2 = listOf("String1", "Int2", "Int3")


Serialisation has nothing to do with Merge, please move such tests to the corresponding class

Ok. I also want to extract common stuff for *Info generation into BaseTestClass.

Good idea. BTW there are already serialisation tests

Please take note on SignatureInfoData and stuff: https://upsource.jetbrains.com/ruby-type-inference/file/1d921936421673c0375ac555967545959bfafbdf/storage-server-api/src/test/java/org/jetbrains/ruby/codeInsight/types/storage/server/impl/SignatureContractSerializationTest.kt?nav=964:981:focused&line=0

I mean they are not ideal, too and worth extracring to some testdata place.

valich · 2017-11-01T11:51:36Z

...er-api/src/main/java/org/jetbrains/ruby/codeInsight/types/storage/server/DatabaseProvider.kt

+
+
+    fun connect(inMemory: Boolean = false,
+                setupConnection: ((Connection) -> Unit)? = null,


I am not sure you need these params. In java classes they were specified just because kotlin did not generate enough overloads for this method. I'd remove all params (except inMemory) and cleanup the code inside

But what about CallStatCompletionTest? It passes additional parameters to DatabaseProvider.

In java classes they were specified just because kotlin did not generate enough overloads for this method

This is what I was talking about, in CallStatCompletionTest I've just re-initialised the default values.

Thx for explanation, now I get it.

valich · 2017-11-07T14:51:05Z

arg_scanner/lib/arg_scanner/type_tracker.rb

+
+    def on_call
+      @submitted_call_counter += 1
+    end


Safe navigation &. FTW
But we'll do it LATER

valich · 2017-11-07T14:52:57Z

`ide-plugin/src/test/java/CallStatCompletionTest.java:100`
concatenating something and absolute path? How should this work? Could you please provide an example?

vladimir-koshelev · 2017-11-07T14:57:44Z

Ofc, it cannot, ty :-)

Add serialization/deserialization for list of `SignatureInfo` Refactor MergeTest.kt Fix misspelling in signature-viwer/build.gradle

Extract code for importing/exporting contract files to `RmcDirectory`. Add a binary serialization test.

- Move serialization tests to ruby-call-signature - Add base class for signature tests which contains common test data - Extract pure serialization tests from merge tests

vladimir-koshelev added 2 commits October 12, 2017 16:49

Add mutex for sending data via socket.

0e93971

Add validation to Infos for checking constrains.

dd72860

vladimir-koshelev requested review from valich and viuginick1 October 16, 2017 13:42

valich mentioned this pull request Oct 26, 2017

Add a Dockerfile #8

Open

valich reviewed Oct 26, 2017

View reviewed changes

valich reviewed Oct 27, 2017

View reviewed changes

valich reviewed Oct 30, 2017

View reviewed changes

valich reviewed Nov 1, 2017

View reviewed changes

valich reviewed Nov 7, 2017

View reviewed changes

vladimir-koshelev added 3 commits November 7, 2017 20:40

Add ARG_SCANNER_PREFIX and ARG_SCANNER_DEBUG ENV vars to arg_scanner

7c57a85

Add SignatureViewer tool

5cdc3e8

Add "*/build/" to gitignore

63b26de

vladimir-koshelev and others added 17 commits November 7, 2017 20:50

Fix commandLine contract server

6e3adae

Replace "javafx.util.Pair" to "kotlin.Pair"

f1f11cf

Remove class name length check from arg_scanner.c

ce86eeb

Refactoring: move db connect calls to DatabaseProvider.

7c051f4

Add SignatureExport to export arg_scanner results to rmc files.

92e2774

Add serialization/deserialization for list of `SignatureInfo` Refactor MergeTest.kt Fix misspelling in signature-viwer/build.gradle

Add SignatureImport tool.

e1548a6

Extract code for importing/exporting contract files to `RmcDirectory`. Add a binary serialization test.

Add patchPluginXml section to ide-plugin/build.gradle.

b0577cd

Rework SignatureViewer. Now it's interactive.

9354cc6

RubyStatTypeProvider: use only return types for type inference.

4759726

Refactor SignatureInfoSerialization

94bcdf0

Refactor SignatureServer

e9d22b3

Refactor RubyStatTypeProviderImpl

777aa95

Refactor constrains in ClassInfo, GemInfo and MethodInfo

ce3cd0c

CallStatCompletionTest change scriptPath to support gradle and ide runs

18d5f68

Remove unused arguments of DatabaseProvider#connect

0c032d4

Refactor SignatureContract tests

1ea6177

- Move serialization tests to ruby-call-signature - Add base class for signature tests which contains common test data - Extract pure serialization tests from merge tests

proper db setup/teardown in plugin completion test

a9b97e9

valich merged commit d5a3bf3 into JetBrains:master Nov 8, 2017


		val testArgs1 = listOf("Int1", "Int2", "Int3")
		val testArgs2 = listOf("String1", "Int2", "Int3")



		fun connect(inMemory: Boolean = false,
		setupConnection: ((Connection) -> Unit)? = null,

Some fixes to run arg_scanner on diaspora specs. #7

Some fixes to run arg_scanner on diaspora specs. #7

Uh oh!

Conversation

vladimir-koshelev commented Oct 16, 2017

Uh oh!

valich commented Oct 26, 2017

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!

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!

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!

valich commented Nov 7, 2017

Uh oh!

vladimir-koshelev commented Nov 7, 2017

Uh oh!

Uh oh!