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

False positive: addition and subtraction on datetime values #1603

Open
rodinaarssen opened this issue May 18, 2022 · 13 comments
Open

False positive: addition and subtraction on datetime values #1603

rodinaarssen opened this issue May 18, 2022 · 13 comments

Comments

@rodinaarssen
Copy link
Member

Describe the bug

The type checker flags addition and subtraction on datetime values as errors.

To Reproduce

datetime hoi = now();
datetime doei = now();
datetime added = hoi + doei;
datetime subtracted = doei - hoi;

Screenshots
image

@PaulKlint
Copy link
Member

Oops, you are completely right: two missing cases. Thanks!

@PaulKlint
Copy link
Member

Solving this in the type checker is trivial (have already fixed it). For the compiler I have to study this a bit more: to my surprise IDateTime does not provide add or subtract operations so their implementation must be hidden in the interpreter somewhere ...

@rodinaarssen
Copy link
Member Author

I should have checked, but it seems that only subtraction is allowed in the interpreter..

rascal>now() + now()
|prompt:///|(8,3,<1,8>,<1,11>): insert into collection not supported on datetime and datetime
Advice: |http://tutor.rascal-mpl.org/Errors/Static/UnsupportedOperation/UnsupportedOperation.html|
ok
rascal>now() - now()
Duration: duration(0,0,0,0,0,0,0)

@rodinaarssen
Copy link
Member Author

protected <U extends IValue> Result<U> subtractDateTime(DateTimeResult that) {

@PaulKlint
Copy link
Member

PaulKlint commented May 18, 2022

Indeed!

This means that I will have to port that code to the runtime of the compiler.

@jurgenvinju
Copy link
Member

Unfortunately yes; this part of the DateTime API is waiting patiently on the introduction of "units and dimensions" in vallang and Rascal, such that datetime - datetime will be an int[Duration] or a real[Duration] in seconds, years, ...

@jurgenvinju
Copy link
Member

When we have that, the code can move into IDateTime, but before that time it seems clunky to move it there. Of course, it is possible, but we'd have to add more "bootstrap" code in terms of manual declarations of data types for the representation of differences.

@PaulKlint
Copy link
Member

Fair enough, let's not go into this rat hole for now :-)

@DavyLandman
Copy link
Member

As part of this rewriting I came across very strange stuff around datetimes.

For example there want is also a datetime library, that also has a duration adt.

@jurgenvinju
Copy link
Member

Yes, way back when we decided that a Duration ADT was a bridge too far for the - operator to produce so I think we settled for a tuple if I recall correctly. For the library that made less sense. Hence we got stuck with two representations for durations. I believe, it was always @mahills perspective that this would be rewritten eventually using a dimension/unit type system.

@DavyLandman
Copy link
Member

DavyLandman commented May 19, 2022

That is what confused me, it's the inverse, the - operator returns a ADT, while the library returns a tuple.

see:

return makeResult(Duration,
VF.constructor(DateTimeResult.duration,
VF.integer(startCal.fieldDifference(endCal.getTime(), Calendar.YEAR)),
VF.integer(startCal.fieldDifference(endCal.getTime(), Calendar.MONTH)),
VF.integer(startCal.fieldDifference(endCal.getTime(), Calendar.DAY_OF_MONTH)),
VF.integer(0),
VF.integer(0),
VF.integer(0),
VF.integer(0)),
ctx);

and:

duration = values.tuple(
values.integer(startCal.fieldDifference(endCal.getTime(), Calendar.YEAR)),
values.integer(startCal.fieldDifference(endCal.getTime(), Calendar.MONTH)),
values.integer(startCal.fieldDifference(endCal.getTime(), Calendar.DAY_OF_MONTH)),
values.integer(0), values.integer(0), values.integer(0),
values.integer(0));

@jurgenvinju
Copy link
Member

thanks for the references. that's exactly right. let's leave it for now until we have the dimensions. otherwise, we'd have yet another breaking change in the library and language when we introduce that.

@DavyLandman
Copy link
Member

True, I would be surprised if many people use this feature, but I agree to leave it as is.

@PaulKlint PaulKlint added enhancement and removed bug labels Mar 17, 2024
@DavyLandman DavyLandman added bug and removed bug labels Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants