Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improve behaviour of inheritance #2220

Merged
merged 2 commits into from
Mar 20, 2022
Merged

Conversation

quentin
Copy link
Member

@quentin quentin commented Mar 16, 2022

Hi, this is a change to improve behavior of components inheritance.

I added some tests.

Instantiate inherited component

It used to be impossible to instantiate components declared by an inherited base:

.comp Base {
  .comp Nested {
  }
}
.comp Derived : Base {
  .init n = Nested
}
============
Error: Referencing undefined component Nested in file foo.dl at line 6
    .init n = Nested
----^----------------

Clauses from base's nested instances

Clauses within nested instances of base components were be missing:

.comp Base {
  .decl rel(s:symbol)
  .comp Nested {
    rel("Nested").
  }
  .init n = Nested
  rel("Base").
}
.init b = Base
---------------
b.rel
s
===============
Base
Nested <------------------ expected
===============

But the clause from Nested would disapear when inherited:

.comp Derived : Base {
  rel("Derived").
}
.init d = Derived
---------------
d.rel
s
===============
Base
<----------------- missing "Nested"
Derived  
===============

Open question: overriding and clauses from nested instance of base

As a side-effect of fixing previous point, I allowed clauses from nested instance of base component to be inherited in the derived component:

.comp Base {
  .decl rel(s:symbol) overridable
  .comp Nested {
    rel("Nested"). // <------ not excluded by Derived .override directive
  }
  .init n = Nested
  rel("Base"). <----- excluded by Derived .override directive
}

.comp Derived : Base {
  .override rel
  rel("Derived").
}
.init d = Derived
---------------
d.rel
s
===============
Nested  <--------------- do we want this entry ?
Derived
===============

Arguments in favor

The rationale for this behavior is that we may want to expect the same result whether .init n = Nested is located in Base or Derived:

.comp Base {
  .decl rel(s:symbol) overridable
  .comp Nested {
    rel("Nested").
  }
  rel("Base").
}

.comp Derived : Base {
  .override rel
  .init n = Nested // <---- now instantiated from Derived
  rel("Derived").
}
.init d = Derived
---------------
d.rel
s
===============
Nested  <--------------- do we want this entry ?
Derived
===============

The second argument is that we get the same behavior when the clause is "orphan" (relation is not visible in a lexical lookup):

.comp Base {
  .comp Nested {
    rel("Nested"). // <------- "orphan" clause
  }
}

.comp Derived : Base {
  .decl rel(s:symbol)
  .init n = Nested
  rel("Derived").
}
.init d = Derived
---------------
d.rel
s
===============
Nested  <--------------- expected
Derived
===============

Opposite argument

The rationale for the opposite behavior would be that a clause relates in priority to the visible relation declaration in a lexical lookup (when one can be found). Hence if there is an override applying to that relation declaration, the clause should be discarded.

Implementing this behavior would be a bit more complicated.

@codecov
Copy link

codecov bot commented Mar 16, 2022

Codecov Report

Merging #2220 (bc8d8dc) into master (6eb3ad0) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2220   +/-   ##
=======================================
  Coverage   75.25%   75.26%           
=======================================
  Files         454      454           
  Lines       27572    27587   +15     
=======================================
+ Hits        20750    20764   +14     
- Misses       6822     6823    +1     
Impacted Files Coverage Δ
src/ast/analysis/ComponentLookup.h 100.00% <ø> (ø)
src/ast/analysis/ComponentLookup.cpp 95.45% <100.00%> (+2.12%) ⬆️
src/ast/analysis/typesystem/TypeConstraints.cpp 90.11% <100.00%> (ø)
src/ast/transform/ComponentInstantiation.cpp 92.39% <100.00%> (+0.02%) ⬆️
src/include/souffle/SouffleInterface.h 88.99% <100.00%> (ø)
...ouffle/datastructure/ConcurrentInsertOnlyHashMap.h 81.30% <0.00%> (-1.63%) ⬇️
src/include/souffle/utility/ParallelUtil.h 83.72% <0.00%> (-1.56%) ⬇️
src/include/souffle/datastructure/LambdaBTree.h 74.04% <0.00%> (+0.76%) ⬆️
src/include/souffle/datastructure/UnionFind.h 97.80% <0.00%> (+2.19%) ⬆️

@b-scholz
Copy link
Member

Is somebody against the proposal?

Could you give a practical example that made you change the component model?

@b-scholz
Copy link
Member

There are examples as part of the PR. Thanks for this.

@b-scholz b-scholz merged commit 11d9be5 into souffle-lang:master Mar 20, 2022
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.

2 participants