megoth commented 4 years ago

I've done some initial work to try to fix type errors that pop up when using the work on the master branch (e.g. for Solid UI it outputs 209 errors).

Doing this I came across a couple of issues that can be split into two groups of problems:

Classes that do not extend their parents properly
Difficulties in combining types that relate to RDFJS and RDFLIB.

Problem 1

For problem 1 I've had a talk with Tim, and he thinks some reasonable fixes are:

To match the signature of Formula#add conform to IndexedFormula#add
To rename IndexedFormula#compareTerm to IndexedFormula#compareTerms

We didn't go into depth of other inconsistencies, but it seems that we will have to break some APIs in any case if we want to fix this properly (currently we've used @ts-ignore to tell TypeScript to ignore them - but this won't fix the problem for projects that depend on this classes, as TypeScript will complain about inconsistent types here as well, which leads to a lot of unneccessary uses of ts-ignore).

Problem 2

Problem 2 seems to come from the fact that many outputs from methods now uses the types described in the RDF/JS Data model specification (I'll just refer to them as RDF/JS types later in this text) instead of the corresponding rdflib ones, and this causes errors for projects such as Solid UI which has worked with rdflib types till now. This has been discussed earlier at https://github.com/linkeddata/rdflib.js/issues/374 when @joepio worked on this earlier (and an even older thread at https://github.com/linkeddata/rdflib.js/issues/137).

The data model types exported from rdflib (e.g. by doing import { NamedNode } from 'rdflib') still refer to rdflib types, which causes type errors for projects such as Solid UI (one fix is to change all type references to the corresponding RDF/JS types, e.g. by doing import { NamedNode } from 'rdflib/src/tf-types', but I think this is a very bad solution.

One simple fix is to make sure that the types that TypeScript refers to are in fact referring to the RDF/JS types. But this will make it more difficult for people to use the extra functionality that is available in the rdflib types.

Now, Tim wants to make sure that types returned from rdflib are using the rdflib types (e.g. so that developers have access to some extra methods, such as toNT, doc, and site).

Since they are a superset, one theory of mine was to allow RDF/JS types as input for all relevant methods, but that they output rdflib types. The idea is that developers who want to work with RDF/JS can still do it (since rdflib types are a superset of RDF/JS types).

I started doing this work on a branch called fix-types-hybrid for those who want to review what I tried doing (note: it's incomplete code, so it won't run and there are bugs that I would've fixed before a proper PR).

When working on the branch I realized I was getting into a mess, and decided to raise these issues to get some feedback on how we might want to go about this.

Another challenge I found wrt types are unions that are different between RDF/JS and rdflib. E.g. for rdflib we have a type that describes which types we allow for many parameters that expect an object (as in subject-predicate-object):

type ObjectType = RDFlibNamedNode | RDFlibLiteral | Collection | RDFlibBlankNode | RDFlibVariable | Empty

While in RDF/JS it looks like the following:

type Quad_Object = NamedNode | BlankNode | Literal | Variable | Term

Please don't mind the different names, the important difference here are the types that don't match in the unions; rdflib allows for Collections and Empty, while RDF/JS allows for Term. I think we might still get the concept to work (allow for RDF/JS types, output rdflib types), but I think it needs a bit of work.

I also tried to work on another branch where the idea was to remove the RDF/JS types altogether, but I realized it was going to require a lot of work and I didn't want to revert the work of @joepio and @fletcher91 before having a discussion on this. For those interested in reviewing that work, it's available on branch fix-types-rdflib.

Hope to get feedback on this from @timbl, @RubenVerborgh, perhaps @dmitrizagidulin and @elf-pavlik who has been involved in these discussions earlier, as well as @joepio and @fletcher91 in case I forgot something in this issue, and @Vinnl who helped me with my thinking on TypeScript types earlier.

RubenVerborgh commented 4 years ago

The data model types exported from rdflib (e.g. by doing import { NamedNode } from 'rdflib') still refer to rdflib types, which causes type errors for projects such as Solid UI (one fix is to change all type references to the corresponding RDF/JS types, e.g. by doing import { NamedNode } from 'rdflib/src/tf-types', but I think this is a very bad solution..

Now, Tim wants to make sure that types returned from rdflib are using the rdflib types (e.g. so that developers have access to some extra methods, such as toNT, doc, and site).

Since they are a superset

Given all of the above, and especially that they are a superset: we should just fix the TypeScript typings such that they are compatible. This is a bug in the typings, nothing more than that.

Since they are a superset, one theory of mine was to allow RDF/JS types as input for all relevant methods, but that they output rdflib types.

Exactly. Or even better perhaps, with generics.

So the question very concretely becomes: what are the points where they are incompatible now, and how do we fix them?

You already gave the example:

type Quad_Object = NamedNode | BlankNode | Literal | Variable | Term

Maybe this is a mistake in RDF/JS and it should rather be "something extending named node / blank node / …", through generics or another mechanism.

timbl commented 4 years ago

(I wonder why ObjectType allows for Empty -- what does that mean? That undefined can be passes as parameter, or that a property on an object can be missing? Just a side wondering)

Tell me how Typescript works. If we declare the QuadObject to be Expression (say), and declare all of Term, NamedNode , BlankNode, Literal, Variable, and also in RDFLIB types Collection, Graph, maybe Set, etc to be subclasses of Expression, does that work>? Or does TS need to have a particular closed set of subclasses of any class to be defined?

timbl commented 4 years ago

The ideal would be that

people using rdflib could have the power of rdflib, with the shortcuts and the flexibility to have quads with all kinds of things as subject, and so on,
those people when they want to parse a typical basic RDF file, use a sparql query, could use RDFJS compatible modules like parsers and serializers
people who work within RDFJS world can use the rdflib modules such as Store, parsers, and the serializers, because it accommodates a superset of their needs. Can we do that using the types?

RubenVerborgh commented 4 years ago

people using rdflib could have the power of rdflib, with the shortcuts and the flexibility to have quads with all kinds of things as subject, and so on,

That should be possible using generics. I.e., if you pass an rdflib object into an rdflib method, it will give you an rdflib object back.

those people when they want to parse a typical basic RDF file, use a sparql query, could use RDFJS compatible modules like parsers and serializers

Check.

people who work within RDFJS world can use the rdflib modules such as Store, parsers, and the serializers, because it accommodates a superset of their needs. Can we do that using the types?

Yes.

@megoth Are you sufficiently familiar with TypeScript to give this ago, or would you need help?

megoth commented 4 years ago

@RubenVerborgh I have enough to give this another go, but am occupied with another task until Thursday. Will try to get on it at once after that, and might contact you if I need some help.

megoth commented 4 years ago

@RubenVerborgh I've given this a new go, but keep running into changes that forces me to change a bunch of files and I get the feeling that I'm reverting a lot of the work that @joepio did to ensure RDF/JS support. As we discussed offline, I hope you're able to give this a go tomorrow, to see if you understand how to go about this.

I've done some work on the fix-types2 branch, that should fix the inconsistencies in classes that extend other classes (i.e. problem 1 described originally) - I haven't verified this changes with Tim, so some more changes on that branch might come.

I've also pushed some partial work to the fix-types2b branch, in case I'm going the right way on it and you want to continue it.

RubenVerborgh commented 4 years ago

@megoth How about 04f7caf0955c22f995c4ecefea3b0c5cc80f7f3b for starters?

Especially check out ad9d672b70e63c1393a05809e26d1e039b844f01 where I am adding tests for the desired typing behavior.

Note that the failing unit tests are due to previous incorrect usage of RDF/JS.

RubenVerborgh commented 4 years ago

One of the tests I wanted to write after bc4beb69095f726b1bdc5bf84f82a3dd9a88280e is (at the top of tests/types/rdfjs-compatibility.ts):

// The rdflib.js DataFactory is an RDF/JS DataFactory
const factory = DataFactory;
const factory2: RdfJs.DataFactory = factory;

However, this fails, the primary reason being that Statement is not an RDF/JS-compatible implementation of Quad, because it relies on the existence of rdflib-specific methods such as substitute. Those methods would need to be rewritten in terms of RDF/JS-compatible operations.

megoth commented 4 years ago

@RubenVerborgh this looks very good, definitely going the right direction.

I'm getting some errors when I try to use it with the current Solid UI:

../rdflib/lib/literal.d.ts:9:22 - error TS2417: Class static side 'typeof Literal' incorrectly extends base class static side 'typeof Node'.
  The types returned by 'fromValue(...)' are incompatible between these types.
    Type 'Literal' is not assignable to type 'T'.
      'Literal' is assignable to the constraint of type 'T', but 'T' could be instantiated with a different subtype of constraint 'FromValueReturns<any>'.

9 export default class Literal extends Node implements TFLiteral {
                       ~~~~~~~

src/acl/add-agent-buttons.ts:189:81 - error TS2345: Argument of type 'Term' is not assignable to parameter of type 'NamedNode | BlankNode | Variable | null | undefined'.
  Type 'Term' is not assignable to type 'Variable'.
    Types of property 'termType' are incompatible.
      Type 'string' is not assignable to type '"Variable"'.

189     const trustedOrigins = trustedApps.flatMap(app => this.groupList.store.each(app, ns.acl('origin')))
                                                                                    ~~~

src/acl/add-agent-buttons.ts:196:49 - error TS2345: Argument of type 'Term' is not assignable to parameter of type 'NamedNode | BlankNode | Variable | null | undefined'.
  Type 'Term' is not assignable to type 'Variable'.

196         const origin = this.groupList.store.any(app, ns.acl('origin'))
                                                    ~~~

src/authn/authn.ts:831:21 - error TS2345: Argument of type 'NamedNode' is not assignable to parameter of type 'string'.

831   const acl = g.sym(aclURI)
                        ~~~~~~

src/authn/authn.ts:832:21 - error TS2345: Argument of type 'NamedNode' is not assignable to parameter of type 'string'.

832   const doc = g.sym(docURI)
                        ~~~~~~

src/widgets/buttons.ts:922:15 - error TS2339: Property 'toNT' does not exist on type 'Term'.

922     p = ps[i].toNT()
                  ~~~~

src/widgets/forms/basic.ts:186:18 - error TS2345: Argument of type 'Node' is not assignable to parameter of type 'Quad_Subject'.
  Type 'Node' is not assignable to type 'Variable'.
    Types of property 'termType' are incompatible.
      Type 'TermType' is not assignable to type '"Variable"'.
        Type '"NamedNode"' is not assignable to type '"Variable"'.

186         is = [st(subject, property, result, doc as Node)]
                     ~~~~~~~

test/custom-matchers/toContainGraph.ts:6:114 - error TS2339: Property 'why' does not exist on type 'Quad<Quad_Subject, Quad_Predicate, Quad_Object, Quad_Graph>'.

6   const diffStatements = expectedStatements.filter(st => !received.holds(st.subject, st.predicate, st.object, st.why as NamedNode))
                                                                                                                   ~~~

test/custom-matchers/toContainGraph.ts:11:83 - error TS2339: Property 'why' does not exist on type 'Quad<Quad_Subject, Quad_Predicate, Quad_Object, Quad_Graph>'.

11 ${receivedStatements.map(st => `- ${st.subject} ${st.predicate} ${st.object} ${st.why} .\n`).join('')}
                                                                                     ~~~

test/custom-matchers/toContainGraph.ts:13:79 - error TS2339: Property 'why' does not exist on type 'Quad<Quad_Subject, Quad_Predicate, Quad_Object, Quad_Graph>'.

13 ${diffStatements.map(st => `- ${st.subject} ${st.predicate} ${st.object} ${st.why} .\n`).join('')}`
                                                                                 ~~~

test/custom-matchers/toEqualGraph.ts:11:83 - error TS2339: Property 'why' does not exist on type 'Quad<Quad_Subject, Quad_Predicate, Quad_Object, Quad_Graph>'.

11 ${receivedStatements.map(st => `- ${st.subject} ${st.predicate} ${st.object} ${st.why} .\n`).join('')}
                                                                                     ~~~

test/custom-matchers/toEqualGraph.ts:13:83 - error TS2339: Property 'why' does not exist on type 'Quad<Quad_Subject, Quad_Predicate, Quad_Object, Quad_Graph>'.

13 ${expectedStatements.map(st => `- ${st.subject} ${st.predicate} ${st.object} ${st.why} .\n`).join('')}`
                                                                                     ~~~

test/custom-matchers/toEqualGraph.ts:16:120 - error TS2339: Property 'why' does not exist on type 'Quad<Quad_Subject, Quad_Predicate, Quad_Object, Quad_Graph>'.

16   const expDiffRecStatements = expectedStatements.filter(st => !received.holds(st.subject, st.predicate, st.object, st.why as NamedNode))
                                                                                                                          ~~~

test/custom-matchers/toEqualGraph.ts:22:83 - error TS2339: Property 'why' does not exist on type 'Quad<Quad_Subject, Quad_Predicate, Quad_Object, Quad_Graph>'.

22 ${receivedStatements.map(st => `- ${st.subject} ${st.predicate} ${st.object} ${st.why} .\n`).join('')}
                                                                                     ~~~

test/custom-matchers/toEqualGraph.ts:24:85 - error TS2339: Property 'why' does not exist on type 'Quad<Quad_Subject, Quad_Predicate, Quad_Object, Quad_Graph>'.

24 ${expDiffRecStatements.map(st => `- ${st.subject} ${st.predicate} ${st.object} ${st.why} .\n`).join('')}`
                                                                                       ~~~

test/custom-matchers/toEqualGraph.ts:27:120 - error TS2339: Property 'why' does not exist on type 'Quad<Quad_Subject, Quad_Predicate, Quad_Object, Quad_Graph>'.

27   const recDiffExpStatements = receivedStatements.filter(st => !expected.holds(st.subject, st.predicate, st.object, st.why as NamedNode))
                                                                                                                          ~~~

test/custom-matchers/toEqualGraph.ts:32:83 - error TS2339: Property 'why' does not exist on type 'Quad<Quad_Subject, Quad_Predicate, Quad_Object, Quad_Graph>'.

32 ${expectedStatements.map(st => `- ${st.subject} ${st.predicate} ${st.object} ${st.why} .\n`).join('')}
                                                                                     ~~~

test/custom-matchers/toEqualGraph.ts:34:85 - error TS2339: Property 'why' does not exist on type 'Quad<Quad_Subject, Quad_Predicate, Quad_Object, Quad_Graph>'.

34 ${recDiffExpStatements.map(st => `- ${st.subject} ${st.predicate} ${st.object} ${st.why} .\n`).join('')}`

Some of them are bugs in Solid UI, but thought it could be useful for you to see the output I currently get. In any case, there are 18 errors altogether, which is better than my 203 errors that I had when I tried fixing this 😼

I'll work on fixing the bugs in Solid UI, and get back to you when I have a list with the remaining errors.

megoth commented 4 years ago

Nevermind, it seems that all bugs were originating in Solid UI. Seems like this is working then, I'll let you know if I find anything else.

RubenVerborgh commented 4 years ago

Good!

In any case, the typings in rdflib.js are over-complex currently. We likely just want to import the RDF/JS typings and take it from there, as opposed to the 3–4 layers of typings that currently exist. It's bound to byte us at some point.

RubenVerborgh commented 4 years ago

@megoth Have the problems been mitigated? Then we might want to close this issue.

megoth commented 4 years ago

@megoth Have the problems been mitigated? Then we might want to close this issue.

Yes, thank you, I think https://github.com/linkeddata/rdflib.js/tree/feature/extended-typing should solve the problems. Perhaps create a PR for it to be merged into master?

I'll close this issue for now, please reopen if necessary.

RubenVerborgh commented 4 years ago

Perhaps create a PR for it to be merged into master?

in https://github.com/linkeddata/rdflib.js/pull/406

linkeddata / rdflib.js

Problems with types #397

Problem 1

Problem 2