Fix URL-to-path conversion for URLs with percent encoding

[s-ludwig]

Falls back to the previous behavior as a fallback in case of encoding errors.

[FreeSlave]

Thanks.

I ran CI and unittests failed on dmd. It doesn't seem to be related to your changes though. Maybe some changes to dmd or phobos caused this. I'll take a look later, and then merge.

[FreeSlave]

The error is caused by the line in the dependency

https://github.com/FreeSlave/inilike/blob/36ffa49318896da29d4c6587af5bb95d75e8b2bd/source/inilike/range.d#L194

Has it become illegal to use byLineCopy on the temporary File and pass it to the function? Looks like some changes to how the refcounted objects are handled or to the order of object destruction.

If I change to

auto f = File(fileName, "r");
return iniLikeRangeReader(f.byLineCopy());

then it works.

It also worked fine without changes on dmd v2.109.1. Could be some regression? Can't find anything related in the changelog for 2.111.0 and 2.110.0

[s-ludwig]

Interesting, it smells a bit like it might be related to lifetime management changes (there were some related to scope and DIP1000 et al., but I don't remember any details). Especially 2.111.0 was a big release though, due to an unusually long backlog on master, and I've also encountered one or two regressions. The File/byLineCopy implementations don't look suspicious w.r.t. reference counting, AFAICS.

I tried to make a reproduction case, but that works without issues on DMD 2.111.0 (posting here, because run.dlang.io's short link generation doesn't work):

import std;

struct S(R) {
    R _r;
    this(R r) { _r = r; }
    bool empty() { return _r.empty; }
    auto front() { return _r.front; }
    void popFront() { _r.popFront(); }
    
    auto byLeadingLines()
    {
        return _r.until!(isGroupHeader);
    }
}

@nogc @safe bool isGroupHeader(scope const(char)[] s) pure nothrow
{
    s = s.stripRight;
    return s.length > 2 && s[0] == '[' && s[$-1] == ']';
}

S!R s(R)(R r) { return S!R(r); }


pragma(msg, "DMD " ~ __VERSION__.stringof);

auto getRange()
{
    return s(File("test.txt", "r").byLineCopy);
}

void main()
{
    auto f = File("test.txt", "w");
    f.write("foo");
    f.close();
    
    foreach (ln; getRange().byLeadingLines)
        writeln("LINE: ", ln);
}

[FreeSlave]

This reproduces the issue for me

import std;

pragma(msg, "DMD " ~ __VERSION__.stringof);

auto iniLikeFileReader(string fileName)
{
    //auto f = File(fileName, "r");
    //return f.byLineCopy();
    
    return File(fileName, "r").byLineCopy();
}

void readIniLike(Reader)(Reader reader) {}

class Handler
{
public:
    this(Range)(Range reader)
    {
        readIniLike(reader);
    }
private:
    string _fileName;
}

unittest
{
    string fileName = "test.txt";
    auto f = File(fileName, "w");
    f.write(`# The first comment`);
    f.close();

    Handler h = new Handler(iniLikeFileReader(fileName));
}

void main() {}

Compile with dmd -unittest main.d

Looks like just passing the range around without even iterating over it can cause the error.

[FreeSlave]

Reported as dlang/dmd#21598

[FreeSlave]

String concatenation behavior has changed as well.
In earlier versions appending an empty slice to the default initialized string resulted in empty non-null string.
Now it results in null string. That's why you had to fix unquoteExec.
Again, can't find anything about it in the changelog. Not sure if it's worth reporting. I guess I shouldn't have relied on this peculiar behavior.

Merging, thanks again.

[s-ludwig]

Yeah, the string concatenation behavior is probably unspecified in that regard, although the previous behavior was quite useful for avoiding redundant checks/flags. Generally, I feel like null checks on arrays always have a slightly bitter taste, as is null vs. .length == 0 is usually a rather subtle difference.